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(legends): legends INNO-630 #206

Merged
merged 5 commits into from
Jun 22, 2017
Merged

feat(legends): legends INNO-630 #206

merged 5 commits into from
Jun 22, 2017

Conversation

degliwe
Copy link
Contributor

@degliwe degliwe commented Jun 21, 2017

  • upgrade component
  • test
  • add documentation

@ghost
Copy link

ghost commented Jun 21, 2017

Your pull request doesn't follow our guidelines. Please fix the following:

  • Pull request title must start with lowercase (?)

Click here for details.

Thank you! 🙏

This comment was made by GitMagic – Magically enforcing your contribution guidelines.

@degliwe degliwe changed the title feat(legends): INNO-630 feat(legends): legends INNO-630 Jun 21, 2017
@ghost
Copy link

ghost commented Jun 21, 2017

Thank you, the title and description now looks good! :bowtie:

Copy link
Contributor

@emeryro emeryro left a comment

Choose a reason for hiding this comment

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

Global overview is good.
Just some small modifications below.

@@ -1 +1,4 @@
# Legends
Copy link
Contributor

Choose a reason for hiding this comment

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

In other components we kept the global title, so I assume we should keep it here too (and set the other title as second level title)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Well let's say like every other component, except button :)
We will have to synchronize here

*/

$form-item-legend-font-size: map-get($ecl-font-size, 'xs');

.legend {
.ecl-form-legend {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure that we have to name class "ecl-form-legend", and not only "ecl-legend". For instance we name other component's classes "ecl-text-input" or "ecl-select"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ex: we have 2 labels components (ecl-label and ecl-form-label) this legend is specific for form element so it seems more future proof and descriptive to stick to ecl-form-legend.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be discussed with the team, as we will have this issue again in the future

<legend class="ecl-form-legend">Legend text comes here</legend>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should put the content into a variable, so we could use composition.

Copy link
Contributor

@emeryro emeryro left a comment

Choose a reason for hiding this comment

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

Review ok

@degliwe degliwe merged commit 8b20678 into master Jun 22, 2017
@degliwe degliwe deleted the feat/legends-INNO-630 branch June 22, 2017 07:24
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.

2 participants