-
Notifications
You must be signed in to change notification settings - Fork 45
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
josepmartins
wants to merge
8
commits into
main
Choose a base branch
from
josepmartins/test-FAQ-deeplink
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
FAQ deep links #171
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
13beabd
Testing id
b936350
Open when location.hash matches id
797f0b5
Merge branch 'main' into josepmartins/test-FAQ-deeplink
fb01738
useHash webhook
e2c685d
Test onchange hash
b9df616
Fix tests
ca54cf4
Merge branch 'main' into josepmartins/test-FAQ-deeplink
88922b2
Add anchor links
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
openattribute 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 theopenattribute to the id of the relevant question?We can then trigger the opening of the question via the URL like:
mywebsite.com?faq=1which opensid="1"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.
I totally agree on not setting the
openattribute 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 theopenattribute to it?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.
To illustrate my comment above, made a commit as an example using the
location.hashto adjust the open boolean in FAQ:6b53e62
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.
🙌 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
windowoperations in auseEffectto mitigate SSR constraints, otherwise works for me 👍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.
I made a
useHashwebhook to handle thewindow.hashoperations, which works pretty finehash-FAQ.mov