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 link sharing to the sharing explainer #1832

Merged
merged 7 commits into from
Jan 5, 2022
Merged

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Dec 17, 2021

closes #1629

I added a test to check that the cross allows to dismiss the modal.
TODO:

  • Update the image to have a high res of it.

Submission checklist:

Layout

  • Change looks good in the desktop web ui
  • Change looks good in the mobile web ui

Theme

  • Components / elements inspected in light mode
  • Components / elements inspected in dark mode

@Tbaut Tbaut added the Status: Review Needed 👀 Added to PRs when they need more review label Dec 17, 2021
@render
Copy link

render bot commented Dec 17, 2021

@render
Copy link

render bot commented Dec 17, 2021

@render
Copy link

render bot commented Dec 17, 2021

@github-actions github-actions bot added the Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes. label Dec 17, 2021
@Tbaut Tbaut changed the title hop Add link sharing to the sharing explainer Dec 17, 2021
cy.intercept("POST", "**/user/store").as("storePost").then(() => {

// dismiss the sharing explainer
sharingExplainerModal.closeButton().click()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sharingExplainerModal.closeButton().click()
sharingExplainerModal.closeButton().safeClick()

I tend to use safeClick for modal closures to remove any chance of "detached from DOM" issues or clicking before an event listener has been registered etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The safeclick is really meant for buttons, as we're checking for the enabled state, which is not available on this cross image. I went back to the click. We can figure it out if we ever have an issue. Safeclick remains kind of a hack that I'd rather use only when needed.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok 👍, just by looking at the test I thought it was a button. I guess to the user it is, at least functions as one, but if the element type is not actually button then yeah no need to use safeClick

@asnaith
Copy link
Member

asnaith commented Dec 18, 2021

Nice addition to the explainer, thanks for adding the new test too 🤩

I left one tiny suggestion and can approve once we get the high res asset :)

@Tbaut Tbaut requested a review from asnaith January 3, 2022 11:25
@Tbaut
Copy link
Collaborator Author

Tbaut commented Jan 3, 2022

Updated the PR with the right image, it's now ready to go

@Tbaut Tbaut enabled auto-merge (squash) January 5, 2022 11:21
@Tbaut Tbaut merged commit e6cb342 into dev Jan 5, 2022
@Tbaut Tbaut deleted the mnt/tbaut-sharing-link-1629 branch January 5, 2022 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed 👀 Added to PRs when they need more review Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the sharing tutorial to talk about links
3 participants