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

refactor(buttons): refactor buttons’ markup and styling - INNO-307 #141

Merged
merged 9 commits into from
May 29, 2017

Conversation

yhuard
Copy link
Contributor

@yhuard yhuard commented May 24, 2017

Closes #86.

Basically, I've imported @emeryro's work and rewritten the template (one template to rule them all). I still need to work on the "variants" (fractal-ly speaking), to provide more concrete examples.

TODO:

  • Add more examples (using fractal's variants): icons, block, etc
  • Provide reference screenshots

# Conflicts:
#	framework/components/ecl-button-groups/_button-groups.scss
#	framework/components/ecl-buttons/_buttons.scss
@kalinchernev kalinchernev self-assigned this May 29, 2017
Copy link
Contributor

@kalinchernev kalinchernev left a comment

Choose a reason for hiding this comment

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

There are first impressions.

Style
See Default button style

# Guidance
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit painful to see that we already know it's unfinished work and someone will have to update urgently at worst moment

@@ -69,6 +67,10 @@ a.breadcrumb__link {
// the breadcrumb segment width.
width: 11px;
color: #fff;

&::before {
Copy link
Contributor

Choose a reason for hiding this comment

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

breadcrumbs europa component library

Is as visible now, the arrow will have to be adjusted a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will see when working on breadcrumbs 👼

Buttons
/**
* Buttons component
* @define button
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this @define ecl-button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, the prefix should not be added in the definition :)

{% endif %}

{% if icon is defined %}
{% set extraClass = extraClass ~ ' ecl-button--' ~ icon %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a comment on this one will be appreciated, it currently seems a bit magical and takes time to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It definitely needs more documentation

@@ -1,354 +1,161 @@
/*
Buttons
Copy link
Contributor

Choose a reason for hiding this comment

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

Pointer cursor missing on hover, not sure if this is desired behavior.

buttons_hover

Copy link
Contributor Author

Choose a reason for hiding this comment

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

W3C: "pointer" The cursor is a pointer that indicates a link.

Copy link
Contributor

Choose a reason for hiding this comment

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

in my face :D

Copy link
Contributor

@kalinchernev kalinchernev left a comment

Choose a reason for hiding this comment

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

I have no other remarks

@yhuard yhuard merged commit 7df4203 into master May 29, 2017
@yhuard yhuard deleted the refactor/buttons-INNO-307 branch May 29, 2017 11:25
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