-
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
base: develop
Are you sure you want to change the base?
PB-877: add swisssearch legacy url limit attribute #1098
Conversation
web-mapviewer Run #3584
Run Properties:
|
Project |
web-mapviewer
|
Run status |
Passed #3584
|
Run duration | 05m 26s |
Commit |
ffa41526da: PB-877: test limit parameter
|
Committer | Felix Sommer |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
21
|
Skipped |
0
|
Passing |
212
|
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.
@@ -147,6 +169,7 @@ const actions = { | |||
* @param {SearchResult} entry | |||
*/ | |||
selectResultEntry: ({ dispatch, getters, rootState }, { entry, dispatcher }) => { | |||
console.log('selectResultEntry', entry) |
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.
Don't use console.log
but log.debug
from the log library
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.
Test link