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

Updates form controls to use the Amsterdam border radius. #3741

Merged
merged 11 commits into from
Sep 8, 2020

Conversation

daveyholler
Copy link
Contributor

@daveyholler daveyholler commented Jul 14, 2020

Summary

This PR updates form controls to use the Amsterdam border radius.

The entirety of mixins/_form.scss was been copied into the Amsterdam theme directory and modified accordingly.

image

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Safari and Firefox
    - [ ] Props have proper autodocs
    - [ ] Added documentation
    - [ ] Checked Code Sandbox works for the any docs examples
    - [ ] Added or updated jest tests
  • 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

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

@cchaos
Copy link
Contributor

cchaos commented Jul 16, 2020

The entirety of mixins/_form.scss was been copied into the Amsterdam theme directory and modified accordingly.

We need to strive to not duplicate mixins that don't actually change any of the internals. This just causes more trouble to maintain all 4 themes. Can you reduce the overriding mixins to just the ones that change for Amsterdam? By simply importing the Amsterdam version of the _form.scss file after the default one should cause the Amsterdam form controls to use the Amsterdam versions. Let me know if you need help though, I could see some possible nested mixins having trouble.

Prepend/Append

These still need more attention because of the rounded corners. They look fine until you make them disabled, invalid, or on focus.

Screen Shot 2020-07-16 at 11 21 44 AM

Screen Shot 2020-07-16 at 11 21 33 AM

Checkbox/Radio

For checkboxes, do we like the increased rounded corners on them or do they start looking too much like radios? Also, should we get rid of the shadows on these as well?

Screen Shot 2020-07-16 at 11 23 02 AM

File picker

The border radius has not been updated for this one.

Screen Shot 2020-07-16 at 11 24 15 AM

Filter buttons

We don't need to tackle this one in this PR, but we should definitely do a quick follow up so we don't have mis-matching styles with the search bar.

Screen Shot 2020-07-16 at 11 26 28 AM

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Aug 3, 2020

It looks like we still have a slight issue with the prepend/appends that I can jump in and fix.

Screen Shot 2020-08-03 at 19 14 03 PM

@daveyholler Would you like me to finish up tackling the other items I mentioned as well like the file picker and checkboxes?

@daveyholler
Copy link
Contributor Author

@cchaos AHHHH. I had a feeling I missed something.

If you want to take over, I won't complain, but I'm also happy to tackle it tomorrow.

@cchaos
Copy link
Contributor

cchaos commented Aug 4, 2020 via email

@daveyholler
Copy link
Contributor Author

@cchaos I believe everything outside of the filter buttons has been addressed now. 🤞

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

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.

Are we keeping the drop shadow on focus?

Screen Shot 2020-08-11 at 10 46 33 AM

One other part that may still need to be addressed is the loading state of the large file picker. It puts an absolute positioned loader bar at the top of the input, but it's not contained within the border-radius.

Screen Shot 2020-08-11 at 10 34 36 AM


As a note, and assuming we're trying to get rid of all the shadows/unnecessary borders to all inputs, there will need to be a follow up PR with updates to:

@daveyholler
Copy link
Contributor Author

Jenkins test this

@kibanamachine
Copy link

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

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.

I pushed a commit that fixes the loading bar of the file picker, removes the dropshadows from checkbox, radios and switches, and moves some border-radius declarations to the mixins.

I tried looking into how to deal with append/prepend stuff better, but went down a bad rabbit hole so I think we can stick with the overrides for now until/if we refactor that bit in JS.

We'll continue to do some follow up styles for other components, but I think this PR is GTG. 👍 🎉

@kibanamachine
Copy link

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

@daveyholler daveyholler merged commit ded27f6 into elastic:master Sep 8, 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.

3 participants