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

Compressed Fixes Round 1 #2338

Merged
merged 15 commits into from
Sep 18, 2019
Merged

Compressed Fixes Round 1 #2338

merged 15 commits into from
Sep 18, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Sep 13, 2019

Fixing some initial issues found while updating Kibana

Added TS defs where missing

There were some compressed props missing from the TS def files of some controls.

Added a doc section for append and prepend

... and fixed up some styling especially if consumer was passing a popover or tooltip. These types of elements didn't get proper styling because the className wasn't being passed down to the right element it would have needed to be pass as the anchorClassName. The docs section is pretty robust showing lots of different types of elements being passed in.

Screen Shot 2019-09-12 at 22 54 46 PM

Checklist

  • Checked in dark mode
  • Checked in mobile
  • Checked in IE11 and Firefox
  • [ ] Props have proper autodocs
  • Added documentation 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

export default () => {
const [isCompressed, setCompressed] = useState(false);
const [isDisabled, setDisabled] = useState(false);
const [isReadOnly, setReadOnly] = useState(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hooks like whaaat!?! 😜

@cchaos cchaos mentioned this pull request Sep 13, 2019
5 tasks
@cchaos
Copy link
Contributor Author

cchaos commented Sep 13, 2019

@ryankeairns I'm not super stoked and the look of these prepend/appends especially in disabled and readOnly states. Would you mind taking a look and let me know if you have any ideas?

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Code, typedef changes LGTM

@ryankeairns
Copy link
Contributor

@cchaos Working through some options, but not finding much...

Aesthetically, I like having no background color and adding a border/divider. That said, I wonder if this results in the actual inputs being less clear or is the bold text + divider on the labels enough to visually separate things?

Screenshot 2019-09-16 13 55 41

I like the disabled state, as-is, having all backgrounds be gray... which feels like the corollary to the all white suggestion in my previous comment:

Screenshot 2019-09-16 14 32 10

For the readonly, I wonder if we can make it not change in height... could a border be added that matches that light background color?

@cchaos
Copy link
Contributor Author

cchaos commented Sep 16, 2019

@ryankeairns Thanks for jamming on that!

I agree after seeing your screenshots that I think the background color is necessary to separate the textual prepends from the input itself... unfortunately.

I will look into adding a slight border to read only states. That might help a lot.

I think just getting your read on the current visuals was what I needed. Like a 👍 or 👎 . So I'm glad you're good with them.

@andreadelrio
Copy link
Contributor

re:Suggest @cchaos This fixes the prepend element but it adds some space to the append item?
image

@andreadelrio
Copy link
Contributor

re:Suggest @cchaos This fixes the prepend element but it adds some space to the append item?
image

Removing this line should fix it https://github.com/elastic/eui/blob/master/src/components/suggest/_suggest_input.scss#L11 I'm guessing we no longer need it.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 16, 2019

Thanks for the reminder to look at EuiSuggest (and I'll do a search for any other prepend/appends). That probably will fix the spacing, but I"m guessing you don't want the darkened background either. I'll take a look.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 17, 2019

@andreadelrio Pushed the fix for the status appends which mainly meant removing the padding and margins.

@snide I also added the .euiFormRow + .euiButton margin

@cchaos
Copy link
Contributor Author

cchaos commented Sep 17, 2019

@ryankeairns I also updated the readOnly state to include a border. I think this helps a ton to make the pre/appends not look so awkward.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 18, 2019

@snide Since you're taking a code day, can you review this one so we can continue the Kibana side?

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Code review looks good. There's a lot of inter component nesting in here, but I don't see a lot of good ways around it. Has me thinking that if we ever modularize things more, we'll need to provide forms, icons and typography as a base core.

@cchaos cchaos merged commit eeb7fb8 into elastic:master Sep 18, 2019
@cchaos cchaos deleted the compressed-fixes-1 branch September 18, 2019 17:53
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.

5 participants