Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(showcase): implementation of reference-block component #735

Conversation

Mallikki
Copy link
Contributor

@Mallikki Mallikki commented Oct 1, 2018

ISSUES CLOSED: #622

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

All components lacked links to their documentation

Issue Number: #622

What is the new behavior?

Stark-ui components now have a link to their documentation in the showcase

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@Mallikki Mallikki requested a review from christophercr October 1, 2018 09:28
@coveralls
Copy link

coveralls commented Oct 1, 2018

Coverage Status

Coverage decreased (-0.6%) to 91.815% when pulling 17e250f on Mallikki:feature/reference-code-component into c6e131c on NationalBankBelgium:master.

@@ -2,6 +2,7 @@
@import "stark-styles.scss";

@import "../app/news/news-component/news.component";
@import "../app/shared/reference-block/reference-block.component";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you put these imports at the bottom of this file in another PR, so maybe you need to put them also at the bottom in this PR too ;)

@@ -0,0 +1,10 @@
<div class="reference-block">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we don't need to add the component class directly in the HTML. You can set it via Angular by using the HostBinding decorator. Basically like this:

export class ReferenceBlockComponent implements OnInit {
	@HostBinding("class")
	public class: string = componentName;
        ...

As we discussed, because of this change, the styles should change since the class is set now in the <stark-reference-block> element See the CSS I propose in my other remark

padding-left: 10px;
padding-top: 10px;
border-radius: 5px;
box-shadow: color(mat-color($grey-palette, 200));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using the 300 shadow instead of 200 so that the reference block stands out of the rest

}
}
.reference-title {
display: inline-flex;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed, I've re-worked these styles to try to keep the same CSS as before, so this is what I got:

.stark-reference-block div {
  background-color: #fff;
  border: solid 1px color(mat-color($grey-palette, 400));
  padding: 10px 10px 10px 50px;
  border-radius: 5px;
  box-shadow: color(mat-color($grey-palette, 300));
  position: relative;

  & mat-icon {
    position: absolute;
    left: 15px;
    top: 14px;
    color: color(mat-color($grey-palette, 700));
  }
  & ul {
    padding: 0;
  }

  & a {
    text-decoration: none;
  }
  & a:hover {
    background-color: $offwhite;
    color: $md-primary;
  }
}

@@ -0,0 +1,10 @@
<div class="reference-block">
<mat-icon svgIcon="file-multiple"></mat-icon>
<div class="reference-title">Reference</div>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified like this: <h2>Reference</h2>

@Mallikki Mallikki force-pushed the feature/reference-code-component branch 2 times, most recently from 148d355 to 12d30c9 Compare October 2, 2018 09:23

& mat-icon {
position: absolute;
display: inline-block;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "inline-block" is not needed anymore

@Mallikki Mallikki force-pushed the feature/reference-code-component branch from 12d30c9 to 69b9d1f Compare October 3, 2018 08:01
@Mallikki Mallikki force-pushed the feature/reference-code-component branch from 69b9d1f to 17e250f Compare October 3, 2018 08:17
@christophercr christophercr merged commit 7b57169 into NationalBankBelgium:master Oct 3, 2018
@Mallikki Mallikki deleted the feature/reference-code-component branch October 16, 2018 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

showcase: create the reference block component
3 participants