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

PB-877: add swisssearch legacy url limit attribute #1098

Merged

Conversation

sommerfe
Copy link
Contributor

@sommerfe sommerfe commented Oct 16, 2024

Copy link

cypress bot commented Oct 16, 2024

web-mapviewer    Run #3796

Run Properties:  status check passed Passed #3796  •  git commit 2a822acfdf: PB-877: fix tests
Project web-mapviewer
Branch Review feat-pb-877-add-swisssearch-legacy-url-limit-attribute
Run status status check passed Passed #3796
Run duration 03m 52s
Commit git commit 2a822acfdf: PB-877: fix tests
Committer Felix Sommer
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 21
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 212
View all changes introduced in this branch ↗︎

@sommerfe sommerfe marked this pull request as ready for review October 16, 2024 13:31
@sommerfe sommerfe requested review from pakb, ltkum and ltshb October 16, 2024 13:31
Copy link
Contributor

@ltshb ltshb left a comment

Choose a reason for hiding this comment

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

I've two comprehension question that should be at least documented on the PR description:

  • Why removing the swisssearch url parameter when it has been entered by the user ? What is the advantage of this ? If I read the ticket associated with this PR, the only issue that we have is that when a user is entering a search text in the search bar the swisssearch is set, so the changes should be very straight forward by not syncing store to url param, but only url param to store.
  • Why adding this strange limit support and how it works ? from where does it come from ? is it documented ? I could not find anything in ticket related to this ? Based on the PR title it is a legacy attribute so it should be in the legacy url param parser or has it been decided to keep this as official ?
    Also the PR title only reflect the limit attribute but not the changes asked in ticket.

src/store/modules/search.store.js Outdated Show resolved Hide resolved
@ltshb
Copy link
Contributor

ltshb commented Oct 17, 2024

I've just seen that this PR contains also #1084 this answer partly my questions above, you should base this PR to #1084 this would ease review and understanding, but then pay attention when merging to merge first #1084 and then rebase this one to develop before merging.

ltshb
ltshb previously requested changes Oct 17, 2024
Copy link
Contributor

@ltshb ltshb left a comment

Choose a reason for hiding this comment

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

See comment in ticket, I think this needs some refinement on how to do it.

@sommerfe sommerfe force-pushed the feat-pb-877-add-swisssearch-legacy-url-limit-attribute branch 6 times, most recently from 6eb79f7 to 9fee35e Compare November 5, 2024 14:52
src/router/storeSync/SearchAutoSelectConfig.class.js Outdated Show resolved Hide resolved
src/router/storeSync/SearchAutoSelectConfig.class.js Outdated Show resolved Hide resolved
src/store/modules/search.store.js Outdated Show resolved Hide resolved
src/store/modules/search.store.js Outdated Show resolved Hide resolved
src/store/modules/search.store.js Outdated Show resolved Hide resolved
src/store/modules/search.store.js Outdated Show resolved Hide resolved
src/api/search.api.js Outdated Show resolved Hide resolved
src/api/search.api.js Outdated Show resolved Hide resolved
@sommerfe sommerfe force-pushed the feat-pb-877-add-swisssearch-legacy-url-limit-attribute branch from 9fee35e to 06ee013 Compare November 5, 2024 15:41
@sommerfe sommerfe requested a review from pakb November 5, 2024 15:41
@sommerfe sommerfe force-pushed the feat-pb-877-add-swisssearch-legacy-url-limit-attribute branch from 06ee013 to 3c6998d Compare November 6, 2024 08:20
@sommerfe sommerfe dismissed ltshb’s stale review November 6, 2024 08:41

Changes were made

Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

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

Some small things to fix, and good to go

src/api/search.api.js Outdated Show resolved Hide resolved
src/router/storeSync/SearchAutoSelectConfig.class.js Outdated Show resolved Hide resolved
src/router/storeSync/SearchAutoSelectConfig.class.js Outdated Show resolved Hide resolved
@sommerfe sommerfe force-pushed the feat-pb-877-add-swisssearch-legacy-url-limit-attribute branch from 3c6998d to e8c5106 Compare November 12, 2024 08:44
@sommerfe sommerfe force-pushed the feat-pb-877-add-swisssearch-legacy-url-limit-attribute branch from e8c5106 to 2a822ac Compare November 12, 2024 08:44
@sommerfe sommerfe requested review from pakb and ltshb November 12, 2024 08:44
@sommerfe sommerfe merged commit 82a17fd into develop Nov 13, 2024
6 checks passed
@sommerfe sommerfe deleted the feat-pb-877-add-swisssearch-legacy-url-limit-attribute branch November 13, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants