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

Add titleSize prop to EuiStep #3340

Merged
merged 15 commits into from
Apr 23, 2020
Merged

Conversation

andreadelrio
Copy link
Contributor

@andreadelrio andreadelrio commented Apr 17, 2020

Summary

Add titleSize prop to EuiStep to allow for custom title sizes.

image

Fixes #3324

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 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_3340/

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Apr 17, 2020

Great start @andreadelrio! I've removed the strikethrough on some PR checklist items that really should be checked/added since this changes the visuals. For instance, mobile should really be checked.

In the docs example you add, I'd write it like a guideline suggesting particular sizes are better than others.

Other things to consider:

  1. Should we add this to the higher component EuiSteps as well so that when passing each step as an object they can change all at once?
  2. Do all title sizes really work? Yes we should be flexible, but do some of them just not work? Like if the title is really small. Or when displaying it at the large title size.

@andreadelrio
Copy link
Contributor Author

In the docs example you add, I'd write it like a guideline suggesting particular sizes are better than others.

@cchaos I actually haven't added an example but I will 🙂

Other things to consider:

  1. Should we add this to the higher component EuiSteps as well so that when passing each step as an object they can change all at once?

This did cross my mind. I think it'd be useful but what to do if they set titleSize for both EuiSteps and EuiStep and they're different values. Would the value for EuiSteps override the one for EuiStep then?

  1. Do all title sizes really work? Yes we should be flexible, but do some of them just not work? Like if the title is really small. Or when displaying it at the large title size.

I think we could exclude the largest and smallest sizes from the options. For all the other sizes, I would think consumers would adjust the size of the body accordingly.

This led me to another thought, would it make sense to add another size for steps (the circle + number)? I'm thinking of 1 more size. I'd look something like this

stepcitos

@cchaos
Copy link
Contributor

cchaos commented Apr 21, 2020

what to do if they set titleSize for both EuiSteps and EuiStep and they're different values. Would the value for EuiSteps override the one for EuiStep then?

The lower level prop, the one on EuiStep would override the overall EuiSteps prop. Giving an example, I want all my steps to be of titleSize = m, however, I want just my last one to be size 'xxs' (for whatever reason). The easiest way to apply this is to apply size m to the whole list, but override the single step inside of the steps object.

This led me to another thought, would it make sense to add another size for steps (the circle + number)?

Would the EuiStep automatically resize the EuiStepNumber based on titleSize or would consumers have to pass that in manually?

@andreadelrio
Copy link
Contributor Author

Would the EuiStep automatically resize the EuiStepNumber based on titleSize or would consumers have to pass that in manually?

@cchaos I think that for flexibility, consumers would pass it manually. Maybe they want smaller titles and smaller circles but maybe they just want smaller titles. In the use case we had, steps were inside a medium flyout so smaller steps would have been nice but I don't think that will always be the case.

@cchaos
Copy link
Contributor

cchaos commented Apr 21, 2020

I think that for flexibility, consumers would pass it manually.

Hmmm, I think this is where flexibility and out-of-the-box good design will clash. I think it's ok for us to make a judgement call that says, "Hey, we are allowing you to change the title size, which allows you to place this at the prominence and hierarchical level that you need, but we're going to make some decisions around how that affects the overall sizing of the elements within."

I think your intuition of needing to adjust the step size with the smaller titles is correct and I would lean towards making it automatic. That way consumers don't also have to think about matching one-to-one since I don't see a very good used case for having mismatched numbers to titles.

@andreadelrio
Copy link
Contributor Author

I think that for flexibility, consumers would pass it manually.

Hmmm, I think this is where flexibility and out-of-the-box good design will clash. I think it's ok for us to make a judgement call that says, "Hey, we are allowing you to change the title size, which allows you to place this at the prominence and hierarchical level that you need, but we're going to make some decisions around how that affects the overall sizing of the elements within."

I think your intuition of needing to adjust the step size with the smaller titles is correct and I would lean towards making it automatic. That way consumers don't also have to think about matching one-to-one since I don't see a very good used case for having mismatched numbers to titles.

@cchaos I think that finding the right balance between flexibility and good design can be tricky. I do see how it'd make sense for us to ensure good out-of-the-box design with smaller titles so I went in this direction: title sizes (xs, s - default and m). Title at size xs will autogenerate smaller steps circles. I added the example too. See:

image

Adjusted icons for status when title is xs too:

image

Note: there's a couple of TS doubts that came up that I'll try to clear up with @thompsongl tomorrow. Mainly I wanted to name the prop the same for EuiSteps and EuiStep but couldn't figure out how to do that here so went with parentTitleSize and titleSize instead.

@kibanamachine
Copy link

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

@andreadelrio
Copy link
Contributor Author

@cchaos I updated the example to get rid of any mention of parentTitleSize and I also reverted the issue that align-items: center; was causing
image

@kibanamachine
Copy link

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

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.

Awesome, so close! Just a few more comments, but really just nit-picky at this point.

src-docs/src/views/steps/steps_example.js Outdated Show resolved Hide resolved
src-docs/src/views/steps/steps_title_sizes.js Outdated Show resolved Hide resolved
src-docs/src/views/steps/steps_example.js Outdated Show resolved Hide resolved
src/components/steps/_mixins.scss Outdated Show resolved Hide resolved
src/components/steps/_mixins.scss Outdated Show resolved Hide resolved
src/components/steps/_step_number.scss Outdated Show resolved Hide resolved
src/components/steps/step_number.tsx Show resolved Hide resolved
@kibanamachine
Copy link

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

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.

🎉 Fantastic! LGTM! I would just make sure to do a quick pass in IE, Firefox, and Safari to make sure they're still good as well.

@kibanamachine
Copy link

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

@andreadelrio andreadelrio merged commit 7778b2f into elastic:master Apr 23, 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.

EuiSteps to allow for different title sizes
4 participants