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
ShareModal: Share link redesign under newDashboardSharingComponent
FF
#87011
ShareModal: Share link redesign under newDashboardSharingComponent
FF
#87011
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just left a few minor questions/suggestions.
Also, I have the same concern as Lucy. Was it validated with the dashboard squad to have the "Share" button as primary and not the "Edit" one?
expect(screen.queryByTestId(selectors.pages.Dashboard.DashNav.shareButton)).not.toBeInTheDocument(); | ||
const newShareButton = await screen.findByTestId(selectors.pages.Dashboard.DashNav.newShareButton.container); | ||
expect(newShareButton).toBeInTheDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity:
- Why are we using
screen.queryByTestId(selectors.pages.Dashboard.DashNav.shareButton)
instead ofscreen.findByText('Share')
as above to retrieve the old share button? - Why are we using
findByTestId
instead ofqueryByTestId
as above to retrieve the new share button?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It's common to change the button text (and in this test, we're not testing that the button has "Share" text). Therefore, the test would fail. Actually, in the middle of the PR I changed "Share Link" to "Share" and the test failed when I was testing another thing.
- FindBy is async. However, the component should be already mounted. I can change it
toolbarActions.push({ | ||
group: 'new-share-dashboard-button', | ||
condition: | ||
config.featureToggles.newDashboardSharingComponent && uid && !isEditing && !meta.isSnapshot && !isPlaying, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this uid && !isEditing && !meta.isSnapshot && !isPlaying
can be extracted into a variable to not be duplicated between the old and the new share buttons.
Yes, it is validated. The wording of the CTA needs to be changed to be only "Share" |
@natydej |
What is this feature?
newDashboardSharingComponent
FF for share modal redesignWhy do we need this feature?
Part of #85240
Which issue(s) does this PR fix?:
Fixes #85241
Special notes for your reviewer:
new-share-button.mov