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

UI: Add a parent level toolbar exclusion key for tabs #18106

Merged
merged 6 commits into from
May 4, 2022

Conversation

JonathanKolnik
Copy link
Contributor

Issue:
Sometimes you might want to exclude tabs altogether but you can't know the specific addon keys that could populate the tabs section of the toolbar ahead of time.

What I did

Added a parent level toolbar exclusion key storybook/tabs that follows the same API as the rest of the toolbar exclusions but filters out all tabs in the toolbar section.

How to test

  • Is this testable with Jest or Chromatic screenshots?

@nx-cloud
Copy link

nx-cloud bot commented Apr 29, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 1ea0c5c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@JonathanKolnik JonathanKolnik changed the title add a parent level toolbar exclusion key for tabs Feature request: add a parent level toolbar exclusion key for tabs Apr 29, 2022
@JonathanKolnik JonathanKolnik requested a review from shilman April 29, 2022 18:24
@shilman shilman changed the title Feature request: add a parent level toolbar exclusion key for tabs UI: Add a parent level toolbar exclusion key for tabs Apr 30, 2022
@shilman shilman added the ui label Apr 30, 2022
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

What about * which is a bit more idiomatic?

Also, does this need to be documented somewhere?

@JonathanKolnik JonathanKolnik force-pushed the jono/ch-1904-excluded-all-tabs-in-toolbar branch from 762af41 to 9e86957 Compare May 1, 2022 23:25
@JonathanKolnik
Copy link
Contributor Author

@shilman do you mean something like tabs/* ?

@shilman
Copy link
Member

shilman commented May 2, 2022

@JonathanKolnik Discussed with the triage team. Initial consensus is that this is a weird API. How about showTabs=false instead to make it more consistent with the other APIs? And if we provide this do we still need the toolbarExclude parameter? If not, can we remove it?

@shilman
Copy link
Member

shilman commented May 2, 2022

While we're at it, we should rename isToolshown to showToolbar and deprecate isToolshown. Then we can remove in 7.0.

@JonathanKolnik JonathanKolnik force-pushed the jono/ch-1904-excluded-all-tabs-in-toolbar branch from 4b286d6 to 4bc8479 Compare May 2, 2022 17:24
@JonathanKolnik JonathanKolnik force-pushed the jono/ch-1904-excluded-all-tabs-in-toolbar branch from 4bc8479 to d50ed19 Compare May 2, 2022 17:42
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@JonathanKolnik small nit, but it looks like the new story looks just like the default story, i.e. there are no tabs to hide so it doesn't really test the feature.

@JonathanKolnik
Copy link
Contributor Author

🤦‍♂️ updated @shilman thank you!

Copy link
Member

@shilman shilman 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 for fixing up the test case @JonathanKolnik -- looking great

@shilman shilman merged commit 1735cd3 into next May 4, 2022
@shilman shilman deleted the jono/ch-1904-excluded-all-tabs-in-toolbar branch May 4, 2022 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants