-
Notifications
You must be signed in to change notification settings - Fork 429
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
fix: feature-specific segments link #4299
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
|
|
|
|
Uffizzi Preview |
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.
This does fix links to feature-specific segments, but it does not enable the "Include Feature-Specific" option. Is that feasible to do as well?
My reasoning is that if you open a link to a segment, there's nothing in the segment itself or the URL that tells you it's feature-specific. If you close the modal intentionally or accidentally, it's more difficult to go back to where you were - especially considering that segment modals do not add browser history, so you can't use Back to re-open them after closing.
Also suggest a different title: fix: Fix links to feature-specific segments not opening the segment details modal
Ok if that's the usecase then it can only be solved by the modal being url based in the history, since segments are paged. I've adjusted the behaviour such that the user can go back and forward in browser history |
Docker builds report
|
@kyle-ssg thanks for the update, I'm able to use browser navigation to get back to a closed segment modal now. One last thing - would it be feasible to have the "Include Feature-Specific" option pre-selected if they open a link to a feature-specific segment? My reasoning is that, to get to a feature-specific segment from the Segments page, you need to have selected the "Include Feature-Specific" option at some point. Having it enabled when you open a link to a feature-specific segment seems more consistent with the navigation behaviour. |
Done @rolodato |
@kyle-ssg I see we're including the This is definitely better than what we have now, but I was hoping to have this parameter added automatically if it's not there. For example, this URL is for a feature-specific segment:
which ends up as:
Instead, I was expecting:
|
I'm not sure how they'd get the url with a missing parameter to begin with. Regardless, this will now update based on the fetched segment. |
It works! One last thing which I'm not sure was present before, is that toggling "Include Feature-Specific" changes the URL and adds a history entry, but navigating back/forward does not actually reflect this change in the page. IMO toggling "Include Feature-Specific" should not add a history change, whether that happens through interaction or not. Or, it should add a history entry but reflect the state change correctly, but I would prefer the former option. Screen.Recording.2024-08-28.at.08.58.47.mov |
In the interest of not keeping PRs around for weeks on end, I've merged this for now. I can adjust the behaviour of this at a later date |
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
Fixes urls to a feature-specific segment.
How did you test this code?