-
Notifications
You must be signed in to change notification settings - Fork 154
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
[#2016] Remove hash symbol from URL when decoding hash #2086
[#2016] Remove hash symbol from URL when decoding hash #2086
Conversation
Googling a bit suggests that this appending of I tested this using the 2324S1 CS2103T RepoSense dashboard using the default view. Some evidence for this hypothesis:
That said, the current fix is good enough to make it work. I propose 2 possible paths in the future:
|
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.
LGTM. As commented above, this is sufficient to fix the bug, but may warrant further refactoring.
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 your inputs @jonasongg and @sopa301. I think this temporary fix is a reasonable solution for now as it resolves the current issue without introducing new issues. We can switch to the Vue Router HTML5 mode in the future.
@jonasongg Since this fix is somewhat unconventional, can you add a comment in the code explaining the rationale behind the change? This will help future developers understand the context and reasoning behind this decision.
Regarding the Cypress tests, we should ideally be adding a test case for the bug fixed in this PR itself. Let us wait to see the progress of PR #2017 over the next few days and make a call over the changes needed.
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.
Nice fix. Agree with @ckcherry23 that a comment to explain the logic for future developers would be useful.
Regarding the tests, I think perhaps @jonasongg could reference the code written in #2017 and add the updated tests, then co-author credit could be added to @nseah21.
@ckcherry23 @MarcusTXK Thanks for the reviews! Just to confirm, I should add the |
@jonasongg #2017 was just merged. You can look into what additions/changes are required on the tests now, thanks! |
@jonasongg After discussion, we decided to merge in #2017, so you can work on the merged test directly on your branch. |
Just added the comment and some test cases! For checked file types, I added some tests to make sure it was testing the last file type to appear in the URL (yml) as that was what was being affected by the bug. Also added some non exhaustive test cases for the code panel options including one for authorshipFileGlob that is most likely the cause for #2058 and #1987. Let me know if I should add more/change anything! |
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.
LGTM! Just have a few minor comments
…ned-after-refresh' into fix-state-not-retained-after-refresh
Should be clarified now! |
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.
LGTM, I think you did a great job
The following links are for previewing this pull request:
|
Fixes #2016
Proposed commit message
Other information
The bug seems to have been caused by the URL containing
#/
at the end, which would cause the last param to not be parsed correctly. I couldn't definitively find what caused the anchor to appear in the URL, so I filtered it out of the URL when decodinghashParams
. Let me know if this behaviour should be changed!Regarding the Cypress tests, #2017 seems to be outdated/some of the tests don't seem to be checking for the correct thing, at least on my machine. If I should investigate this further in anticipation of its being merged, let me know as well!