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

[EuiFilterButton] Fix Amsterdam style #3591

Merged
merged 5 commits into from
Jun 11, 2020

Conversation

phyolim
Copy link
Contributor

@phyolim phyolim commented Jun 10, 2020

Summary

before:
Screen Shot 2020-06-10 at 1 13 57 PM
Screen Shot 2020-06-11 at 2 37 06 PM

after:
Screen Shot 2020-06-11 at 11 36 14 AM
Screen Shot 2020-06-11 at 2 21 58 PM

fixes #3590

Checklist

  • Check against all themes for compatibility in both light and dark modes
    - [ ] Checked in mobile
  • Checked in IE11 and Firefox
    - [ ] Props have proper autodocs
    - [ ] Added documentation
    - [ ] Checked Code Sandbox works for the any docs examples
    - [ ] Added or updated **[jest tests](https://github.com/elastic/eui/blob/master/wiki/testing.md)**
    - [ ] Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 10, 2020

💚 CLA has been signed

@cchaos
Copy link
Contributor

cchaos commented Jun 10, 2020

Welcome @phylim! Would you please sign the CLA linked below. Be sure to use the same email address linked to your Github account.

@phyolim
Copy link
Contributor Author

phyolim commented Jun 11, 2020

Signed it and updated the PR based on your comments on the issue.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks @phylim. There are a couple other best-practices we try to adhere to when making PR's.

  1. Add a description explanation of the path taken to solve the issue in the PR summary (even if it might be repeating a discussion that was had in the issue)
  2. Add before and after screenshots in the PR summary
  3. Even though this is a simple style fix, there are still checklist items that are appropriate which I have removed the strikethrough from. Please be sure to check these.

I will get Jenkins (our CI tool) to kick off a build and run the tests once those items above and the review comments have been addressed.

@@ -0,0 +1,3 @@
.euiButtonEmpty {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately you can't apply the style this way because it will affect all EuiButtonEmpty instances not just the one created by the filter groups. You will need to target the custom class passed in EuiFilterButton called .euiFilterButton.

phyolim added 2 commits June 11, 2020 14:11
box-shadow looks different than form field. @cchaos said it would be fixed when SASS mixins are updated.
@phyolim phyolim force-pushed the amsterdam-filter-group branch from 9c59074 to 040aebf Compare June 11, 2020 18:11
phyolim and others added 2 commits June 11, 2020 14:41
 (moved to Amsterdam section and added link to PR)
@cchaos
Copy link
Contributor

cchaos commented Jun 11, 2020

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3591/

@phyolim
Copy link
Contributor Author

phyolim commented Jun 11, 2020

@cchaos Thanks for your patience and details in explanations.
This is my first attempt to contribute on an open source project and it's very helpful.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Not a problem, happy to have you contribute!

Just one more advice for how we use selectors.

src/themes/eui-amsterdam/overrides/_filter_group.scss Outdated Show resolved Hide resolved
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@cchaos
Copy link
Contributor

cchaos commented Jun 11, 2020

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3591/

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

🎉 LGTM. Thanks @phylim

@cchaos cchaos changed the title added overwrite for amsterdam euiFilterGroup css [EuiFilterButton] Fix Amsterdam style Jun 11, 2020
@cchaos cchaos merged commit 402f1e1 into elastic:master Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Amsterdam euiFilterGroup have a sharp angle border on rounded buttons
3 participants