-
Notifications
You must be signed in to change notification settings - Fork 149
Fix csp violation breaking “back to previous page” link #2233
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
Fix csp violation breaking “back to previous page” link #2233
The head ref may contain hidden characters: "dev/issue-2232-Fix-CSP-Violation-Breaking-\u201CBack-to-Previous-Page\u201D-Link"
Conversation
…ass selector for all files using it for back navigation
|
Hi @Eashana0104! Thank you so much for your contribution and congratulations on your first PR to AtoM! I'm know you are getting your development environment set up still, but the Prettier test failed. Everything else passed! |
melaniekung
left a comment
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.
Hi @Eashana0104 , thanks for the contribution! Just added some small feedback, but otherwise looks great!
| <a href="javascript:history.go(-1)"> | ||
| <?php echo __('Back to previous page.'); ?> | ||
| </a><br> | ||
| <a href="#" class="js-back-link" data-fallback-url="<?php echo url_for('@homepage'); ?>"><?php echo __('Back to previous page'); ?></a><br> |
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.
We should use a data-action like data-action="back" instead of a class to call the js function for better readability. (Same for all the other php classes)
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.
This does increase the readability. I have resolved this in my commits and formatted the closing tags properly to increase readability further!
js/historyNavigation.js
Outdated
| }); | ||
|
|
||
| function historyNavigation() { | ||
| document.querySelectorAll('.js-back-link').forEach(function (el) { |
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.
Using a data-action will require updating the querySelectorAll here, but should work with the rest of the function as is.
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.
This makes sense in accordance to the change on selector. Resolved by 52deaf9
| import "/js/deletePhysicalStorage"; | ||
| import "/js/settingsFindingAid"; | ||
| import "/js/refreshJobs"; | ||
| import "/js/historyNavigation"; |
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.
Daniel has mentioned to us that you are having troubles setting up prettier checks so I thought it may be useful to add this - I believe the Prettier test may be failing due to the missing empty line 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.
The Prettier check was failing due to some formatting requirement failing in historyNavigation.js. i have fixed that using the command prettier --write js/historyNavigation.js. Now all the .js files are formatted according to Prettier rules. Resolved by 5f7db10
…“Back-to-Previous-Page”-Link
…ious-Page”-Link' of https://github.com/Eashana0104/atom into dev/issue-2232-Fix-CSP-Violation-Breaking-“Back-to-Previous-Page”-Link
…er readability" This reverts commit 8f51f8f.
melaniekung
left a comment
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.
Thanks for the updates @Eashana0104 , this looks good to me! Congrats on completing your first PR to AtoM!
anvit
left a comment
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.
Added a couple small comments
js/historyNavigation.js
Outdated
| "use strict"; | ||
|
|
||
| $(() => { | ||
| historyNavigation(); |
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 would suggest putting the body of the entire function here instead of a single named function call
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.
Resolved by b84114c
js/historyNavigation.js
Outdated
| window.history.back(); | ||
| } else { |
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.
It would be cleaner to just return; after the window.history.back(); and skip the else entirely.
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.
Resolved by 0476724
anvit
left a comment
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.
Thanks @Eashana0104 ! This looks good!
…“Back-to-Previous-Page”-Link
Closes #2232
Changes Made -
Clicking on

Back to previous pageNo more CSP compliance errors on clicking

Back to previous pageLeads back to the previous page in history.
