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

FAQ deep links #171

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

FAQ deep links #171

wants to merge 8 commits into from

Conversation

josepmartins
Copy link
Contributor

@josepmartins josepmartins commented Feb 1, 2023

Summary

This PR showcase and create a storybook example of the FAQ items using anchored links.

Tracking issue: https://github.com/github/primer/issues/1749

👀 Preview links

hash-FAQ.mov

List of notable changes:

What should reviewers focus on?

Steps to test:

Supporting resources (related issues, external links, etc):

Contributor checklist:

  • All new and existing CI checks pass
  • Tests prove that the feature works and covers both happy and unhappy paths
  • Any drop in coverage, breaking changes or regressions have been documented above
  • New visual snapshots have been generated / updated for any UI changes
  • All developer debugging and non-functional logging has been removed
  • Related issues have been referenced in the PR description

Reviewer checklist:

  • Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
  • Check that tests prove the feature works and covers both happy and unhappy paths
  • Check that there aren't other open Pull Requests for the same update/change

Screenshots:

Please try to provide before and after screenshots or videos

Before After

@changeset-bot
Copy link

changeset-bot bot commented Feb 1, 2023

⚠️ No Changeset found

Latest commit: 88922b2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2023

🟢 No design token changes found

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2023

🟢 No visual differences found

Our visual comparison tests did not find any differences in the UI.

@josepmartins josepmartins temporarily deployed to github-pages February 1, 2023 09:40 — with GitHub Actions Inactive
@josepmartins josepmartins self-assigned this Feb 1, 2023
</p>
</FAQ.Answer>
</FAQ.Item>
<FAQ.Item open id="test">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the drive-by comment @josepmartins - this looked interesting and I was also thinking about this yesterday so hope you don't mind me dropping some feedback here? 😬

I think in practice we'd typically not want to set open attribute by default in the markup, but rather only when required via a "deeplink" url. What do you think about passing a query param, which FAQ component reads, parses and then adds the open attribute to the id of the relevant question?

We can then trigger the opening of the question via the URL like:

mywebsite.com?faq=1 which opens id="1"

Copy link
Contributor Author

@josepmartins josepmartins Feb 1, 2023

Choose a reason for hiding this comment

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

I totally agree on not setting the open attribute by default (we should document this). This was more of a test to showcase how the feature "should look", that's why I forced the open attribute.

Not sure about the need for a query param here (or maybe I didn't understand your proposal lol). Wouldn't it be better to use a normal anchor link (mywebsite.com#whats-the-meaning-of-life) that scrolls down to the id of the question and the FAQ component reads, parses, and adds the open attribute to it?

Copy link
Contributor Author

@josepmartins josepmartins Feb 1, 2023

Choose a reason for hiding this comment

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

To illustrate my comment above, made a commit as an example using the location.hash to adjust the open boolean in FAQ:

6b53e62

Copy link
Collaborator

Choose a reason for hiding this comment

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

🙌 I love that idea. Glad we're aligned on the optional attribute. I think your approach of doing it without query params makes total sense too. Just bear in mind that the component will need to run any window operations in a useEffect to mitigate SSR constraints, otherwise works for me 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a useHash webhook to handle the window.hash operations, which works pretty fine

hash-FAQ.mov

@josepmartins josepmartins temporarily deployed to github-pages February 1, 2023 13:17 — with GitHub Actions Inactive
@josepmartins josepmartins force-pushed the josepmartins/test-FAQ-deeplink branch from fdd3e2a to 6b53e62 Compare February 1, 2023 13:21
@josepmartins josepmartins temporarily deployed to github-pages February 1, 2023 13:28 — with GitHub Actions Inactive
@josepmartins josepmartins force-pushed the josepmartins/test-FAQ-deeplink branch from 6b53e62 to b936350 Compare February 1, 2023 14:34
@josepmartins josepmartins temporarily deployed to github-pages February 1, 2023 14:42 — with GitHub Actions Inactive
@josepmartins josepmartins temporarily deployed to github-pages February 1, 2023 15:06 — with GitHub Actions Inactive
@josepmartins josepmartins force-pushed the josepmartins/test-FAQ-deeplink branch from 650d6bc to e2c685d Compare February 1, 2023 16:11
@josepmartins josepmartins temporarily deployed to github-pages February 1, 2023 16:19 — with GitHub Actions Inactive
@josepmartins josepmartins temporarily deployed to github-pages February 2, 2023 10:26 — with GitHub Actions Inactive
@josepmartins josepmartins changed the title Test FAQ with anchored links Deep links in FAQ Feb 6, 2023
@josepmartins josepmartins temporarily deployed to github-pages February 6, 2023 15:22 — with GitHub Actions Inactive
@josepmartins josepmartins force-pushed the josepmartins/test-FAQ-deeplink branch 3 times, most recently from dc7a6eb to 895686d Compare February 7, 2023 11:49
@josepmartins josepmartins changed the title Deep links in FAQ FAQ deep links Feb 7, 2023
@josepmartins josepmartins temporarily deployed to github-pages February 7, 2023 11:57 — with GitHub Actions Inactive
@josepmartins josepmartins force-pushed the josepmartins/test-FAQ-deeplink branch from 895686d to ebce165 Compare February 7, 2023 13:12
@josepmartins josepmartins temporarily deployed to github-pages February 7, 2023 13:20 — with GitHub Actions Inactive
@josepmartins josepmartins force-pushed the josepmartins/test-FAQ-deeplink branch 2 times, most recently from 2652563 to 8ceeb5c Compare February 7, 2023 16:36
@josepmartins josepmartins temporarily deployed to github-pages February 7, 2023 16:46 — with GitHub Actions Inactive
@josepmartins josepmartins force-pushed the josepmartins/test-FAQ-deeplink branch from 8ceeb5c to 88922b2 Compare February 7, 2023 17:06
@josepmartins josepmartins removed their assignment Oct 9, 2023
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