-
Notifications
You must be signed in to change notification settings - Fork 11
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
PB-877: add swisssearch legacy url limit attribute #1098
Conversation
web-mapviewer Run #3796
Run Properties:
|
Project |
web-mapviewer
|
Branch Review |
feat-pb-877-add-swisssearch-legacy-url-limit-attribute
|
Run status |
Passed #3796
|
Run duration | 03m 52s |
Commit |
2a822acfdf: PB-877: fix tests
|
Committer | Felix Sommer |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
21
|
Skipped |
0
|
Passing |
212
|
View all changes introduced in this branch ↗︎ |
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'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 theswisssearch
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.
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.
See comment in ticket, I think this needs some refinement on how to do it.
6eb79f7
to
9fee35e
Compare
9fee35e
to
06ee013
Compare
06ee013
to
3c6998d
Compare
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.
Some small things to fix, and good to go
3c6998d
to
e8c5106
Compare
e8c5106
to
2a822ac
Compare
Test link