-
Notifications
You must be signed in to change notification settings - Fork 15
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
WFNEWS-2419 route back to map page from full details #2167
Conversation
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 of this logic seems a bit contrived. Can we simplify or clarify it a bit better?
if (this.params?.['source']) { | ||
// use query params to determine the layer, coordinates and zoom level for routing back to the map | ||
if ((this.params['source'] === 'map' || this.params['source']?.[0] === 'map') | ||
&& (this.params?.['type'] === 'area-restriction' || this.params?.['type'] === 'bans-prohibitions' | ||
|| this.params?.['type'].includes('evac')) | ||
) { |
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.
How would we summarize this logic into plain english?
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.
If the user accessed the full details page from the map, and if the full details page contains one of area restrictions, fire bans, evac orders or evac alerts, use the backToMap function to route back to the map using the same layer and zoom level along with the appropriate coordinates for that type.
I updated the comments in the code with this explanation
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.
Logic updated to make it more readable
if ((this.params?.['source'] === 'map' || this.params?.['source']?.[0] === 'map') | ||
&& (this.params?.['type'] === 'area-restriction' || this.params?.['type'] === 'bans-prohibitions' | ||
|| this.params?.['type'].includes('evac')) | ||
) { |
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.
Same applies here
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.
Logic updated to make it more readable
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.
Looks good, It did not mentioned in the ticket but I assume we want to do the same thing for Danger Rating? I think back button should take user to map with zoom level too. Can you confirm with Christine?
…n more gracefully
// If the user accessed the full details page from the map, and if the full details page contains one of area restrictions, | ||
// fire bans, evac orders or evac alerts, use the backToMap function to route back to the map using the same layer and zoom level, | ||
// along with the appropriate coordinates for that type | ||
if (this.navigatedfromMapPage() && this.isCurrentlyNavigatedOn()) { |
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.
if (this.navigatedfromMapPage() && this.isCurrentlyNavigatedOn()) { | |
if (this.navigatedfromMapPage() && this.isCurrentlyNavigatedOn(['area-restriction', 'bans-prohibitions', 'evac-orders', 'evac-alerts', 'danger-rating'])) { |
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.
Done
isCurrentlyNavigatedOn() { | ||
const fullDetailPageTypes = ['area-restriction', 'bans-prohibitions', 'evac-orders', 'evac-alerts', 'danger-rating']; | ||
if(fullDetailPageTypes.includes(this.params?.['type'])){ |
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.
isCurrentlyNavigatedOn() { | |
const fullDetailPageTypes = ['area-restriction', 'bans-prohibitions', 'evac-orders', 'evac-alerts', 'danger-rating']; | |
if(fullDetailPageTypes.includes(this.params?.['type'])){ | |
isCurrentlyNavigatedOn(pageList) { | |
if(pageList.includes(this.params?.['type'])){ |
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.
Done
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.
Updated to make more generic and clear what's being checked.
Add coordinates and zoom level to query params when routing to the full details page in order to return the user to where they were just at on the map.
Also updating desktop version to route back to previous zoom level on the map for bans, evacs, area restrictions, rather than use a hardcoded value.
Includes some formatting updates applied automatically in VSCode.