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.

Sign up for GitHub

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

Jump to bottom

[Storybook] Add stories for more components (letters R-S) - Part 2 #7728

Merged
merged 11 commits into from
May 10, 2024
Merged

[Storybook] Add stories for more components (letters R-S) - Part 2 #7728

merged 11 commits into from
May 10, 2024

Conversation

mgadewoll
Copy link
Contributor

@mgadewoll mgadewoll commented May 3, 2024 •
edited
Loading

Summary

Relates to #7482

This PR adds more stories for components with the letter S:

  • EuiSelectableList
  • EuiSelectableListItem
  • EuiSelectableSearch
  • EuiSelectableMessage
  • EuiSelectableTemplateSidewide

Additionally adds base VRT reference images based on the added stories.

QA

  • Confirm that the prop controls for all the below stories provide relevant controls and that those controls work as expected:

@mgadewoll mgadewoll added documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog labels May 3, 2024
@mgadewoll mgadewoll marked this pull request as ready for review May 3, 2024 08:41
@mgadewoll mgadewoll requested a review from a team as a code owner May 3, 2024 08:41
cee-chen
cee-chen reviewed May 8, 2024
View reviewed changes
src/components/selectable/selectable_message/selectable_message.stories.tsx Outdated
Copy link
Contributor

@cee-chen cee-chen May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

On today's episode of yet another "I had no idea we were exporting this":

A very handsome man looking confused and asking 'But why?'

cee-chen
cee-chen approved these changes May 8, 2024
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

To be totally honest I didn't look too closely at the EuiSelectable subcomponent stories because I hadn't realized we were exporting them, I'm not sure who's even using them, and their props are a bit of a nightmare 🙈

@cee-chen
Copy link
Contributor

cee-chen commented May 8, 2024

I'd also be very open to just removing those stories entirely (EuiSelectableMessage, EuiSelectableList, and EuiSelectableSearch specifically). Frankly I don't think we should be exporting or documenting them and their controls is helpful to consumers and might be actively confusing. I'll leave that up to you though!

@mgadewoll
Copy link
Contributor Author

I'd also be very open to just removing those stories entirely (EuiSelectableMessage, EuiSelectableList, and EuiSelectableSearch specifically). Frankly I don't think we should be exporting or documenting them and their controls is helpful to consumers and might be actively confusing. I'll leave that up to you though!

@cee-chen Looking at Kibana alone, I do see imports of the components though. Mainly for testing purposes but also in code usage (e.g. for EuiSelectableMessage and EuiSelectableListItem)
Imho, if we export a component we should document it. I understand your point that those components would likely only be used in composition as EuiSelectable anyway but as it's a rather complex component, maybe it's not a bad thing to have them standalone in Storybook for testing and documentation? Especially for EuiSelectableMessage and EuiSeletableSearch I'd think it's still useful as those components are not always visible in EuiSelectable.
And though it's not expected, maybe there might be use cases for the subcomponents to be used separately?

I'd personally keep the stories - at least for now. If we notice down the line that they are obsolete or harmful to comprehension we could still remove them. Wdyt?

@mgadewoll mgadewoll force-pushed the storybook/7482-stories-r-s-part-2 branch from ef4c8c2 to b98f015 Compare May 10, 2024 08:27
@mgadewoll
Copy link
Contributor Author

ℹ️ These two commits from @cee-chen [1, 2] are added back after accidentally removing them by rebasing 🙈

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@mgadewoll mgadewoll merged commit bd992c1 into elastic:main May 10, 2024
5 checks passed
@cee-chen
Copy link
Contributor

That's very valid, I always forget to actually check Kibana 😬 And yes, was totally good leaving their storybooks in just to clarify!

tkajtoch added a commit to tkajtoch/eui that referenced this pull request May 17, 2024
@tkajtoch
Revert "[Storybook] Add stories for more components (letters R-S) - P…
1208d9a 
…art 2 (elastic#7728)"

This reverts commit bd992c1.
@tkajtoch tkajtoch mentioned this pull request May 17, 2024
Merged
@cee-chen cee-chen mentioned this pull request May 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen approved these changes

Assignees
No one assigned
Labels
documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mgadewoll @cee-chen @kibanamachine @elasticmachine