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 support for queryParam flexibility #367

Merged
merged 5 commits into from
Jun 4, 2024
Merged

Conversation

MelSumner
Copy link
Member

@MelSumner MelSumner commented May 29, 2024

If merged, this PR adds a conditional for excludeAllQueryParams which is set to false by default. If set to true then it should check for the presence of queryParams and if QPs are present, don't manage the route transition or focus.

  • updated README
  • extra tests have been added
  • a little extra link to browser tests has been added to the dummy app for dev env convenience

Tests passing:
CleanShot 2024-06-04 at 13 37 43@2x

Base automatically changed from melsumner/add-js-doc-comments to main June 4, 2024 15:13
@MelSumner MelSumner marked this pull request as ready for review June 4, 2024 15:14
@MelSumner MelSumner changed the title [WIP] Add support for queryParam flexibility Add support for queryParam flexibility Jun 4, 2024
Add TODO comment

Updated tests, worked on logic (2 failing tests at this point)

Co-authored-by: Joseph D. Sumner <me@jds.dev>

make the new test names more explicit

set this.transition bc we can't pass an arg to a getter

getting closer

updated tests

updated the mock in the test, tests pass

add a tests link in the dummy app for easier development

these comments will come  out once I am confident this is correct
@MelSumner MelSumner requested a review from a team June 4, 2024 19:31
Copy link
Contributor

@jgwhite jgwhite left a comment

Choose a reason for hiding this comment

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

I can certainly imagine situations in which this would be useful. The structure looks good to me. I think the argument name is reasonable, given that it’s quite an esoteric need. One minor stylistic suggestion, but apart from that 👍

addon/components/navigation-narrator.js Outdated Show resolved Hide resolved
Simplify conditional logic

Co-authored-by: Jamie White <jamie@jgwhite.co.uk>
@MelSumner MelSumner merged commit 7683260 into main Jun 4, 2024
1 check passed
@MelSumner MelSumner deleted the melsumner/add-qp-support branch June 4, 2024 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide: should we add more flexibility for the way queryParams are handled for focus resets?
3 participants