Updating browser history integration with the navigation Table of Contents (ToC) filter #847
Conversation
|
I wonder if the maintainers could pass the linter test for me , as the linter does not fix the issues in windows . |
|
Thank you @RONAK-AI647! I will invite my colleagues for review. |
|
Hi @RONAK-AI647 I am not sure whether this is within the scope if this task but I did pick up the issue below; Directly accessing the for example Screen.Recording.2024-12-02.at.15.50.56.mov |
|
@akolson I see it , but it works when you paste the extension "/kcard?filter=kcard" rather than " /?filter=kcard ". because the URL was suppposed to be this , according to the box below.
If you say that pasting this URL extension " /?filter=kcard " does not enter the button's name in Toc filter , yes it does not , I see that . |
|
@RONAK-AI647 unfortunately I can also replicate this
You can check here on your pr deployment Screen.Recording.2024-12-02.at.18.59.36.mov |
|
@akolson , this could be beyond the scope of this task , i can be wrong , if you all confirm it as an emerging issue , i will work on that as well. |
|
Hi @RONAK-AI647!! Thanks for working on this issue :). Yes, this issue is in the scope of this PR. Previous to this PR we didnt had a A couple of things that can be originating this error:
Compartir.pantalla.-.2024-12-03.07_01_56.mp4
We need to push the filter as query debounced by 2s (we can play with the wait time if needed), and we need to read these chages so when we reload the page or go back we have this value set. After we push the query, we dont need to do any of this https://github.com/RONAK-AI647/kolibri-design-system/blob/ac409f6b908ff90f9487bba17bb784c3aa11867e/docs/common/DocsPageTemplate/SideNav/index.vue#L99-L106 because the browser history is the one that will be in charge of managing the state changes, so we dont need to listen to the pop. Hope these instructions help. Let me know if you have any questions :). |
|
Thank You @AlexVelezLl for the guidance .🥰 |
Screen.Recording.2024-12-05.231831.mp4Not loosing filter query when page reloads . @AlexVelezLl |
|
Hey members , take a look please. @AlexVelezLl @akolson
Screen.Recording.2024-12-06.232940.mp4
Screen.Recording.2024-12-06.233013.mp4
Screen.Recording.2024-12-06.233057.mp4 |
There was a problem hiding this comment.
Thank you @RONAK-AI647!! This is looking great! We are almost there, the only thing I have noticed that is missing is that after we click on a component, we dont preserve the filter query and it gets deleted, this is the only behaviour missing from the table that was presented in the issue:
| URL | action |
|---|---|
/ |
first URL |
/?filter=button |
after enter 'button' into ToC filter |
/kbutton?filter=button |
after clicking KButton entry |
/?filter=button |
browser back |
/ |
browser back |
Its the 3rd action what is still missing. Apart from that everything looks good!
|
Also, can you please rebase your branch on top of the latest KDS develop? We recently updated the KDS nodejs version to v18 and vue to v2.7, and we need to do the rebase for the actions to run successfully :). There are also new linting rules, so after rebasing, please run |
This reverts commit 7ee6049.
RONAK-AI647
force-pushed
the
navigating-browser-history-issue
branch
from
f9fe853 to
31df512
Compare
|
@AlexVelezLl I have considered the 3rd step and now its preserved. You can take a look . Screen.Recording.2024-12-11.210359.mp4 |
|
@MisRob @AlexVelezLl , I face a problem here , i rebased the pr , the lint fix yet shows list of errors in the terminal. Here the common cicd checks also are failing , any idea , what is wrong .
Screen.Recording.2024-12-11.211223.mp4 |
|
Hey @RONAK-AI647! Did you run the |
yes, I did yarn install. , yet the yarn lint-fix does not work I don't understand why these tests fail. do i open a new PR??? |
|
Hey @RONAK-AI647! Its possible that the error you are facing is because of a specific setting in your computer. Can you explore the answers described in this thread: https://stackoverflow.com/questions/8965606/node-and-error-emfile-too-many-open-files. I was able to run I will further investigate the failing netlify actions, these shouldnt be failing. |
| } | ||
|
|
||
| // Modify navigation links to preserve the filter query | ||
| this.$nextTick(() => { |
There was a problem hiding this comment.
I would suggest better update NavLink and DocsLibraryLink to pass the query params, so we have dont have to add an event listener to all links of the page :)
|
@AlexVelezLl i got the linting problem and the conflicts with rest of the checks will be dealt if i open a new PR . The points you mentioned , i will keep in mind and soon i will a new PR with everything resolved. Thank you |
|
I managed to get it working @RONAK-AI647! Here is the deployed site https://deploy-preview-847--kolibri-design-system.netlify.app/. You wont need to open a different PR 👐. |
super Thank You !!!!!! |
…/RONAK-AI647/kolibri-design-system into RONAK-A1647#213-Improve-browser-history-integration-with-the-navigation-Table-of-Contents-(ToC)-filter
|
@AlexVelezLl I updated the navlink.vue and DocsLibraryLink.vue as you said , take a look please. |
There was a problem hiding this comment.
Hey @RONAK-AI647! This looks good to me! We are almost there, just suggested to remove the debounceUpdateQuery method as it seems is not needed anymore, since the intention of the debounce has been addressed in another way. After this, we can merge the PR. Thank you!
| query: { ...this.$route.query, filter: newValue }, | ||
| }); | ||
| } | ||
| this.debouncedUpdateQuery(newValue); |
There was a problem hiding this comment.
As you took another approach with the popstate listener, lets just remove this debouncedUpdateQuery call. As there we just do the same logic of pushing the query.
| this.debouncedUpdateQuery(newValue); |
| window.sessionStorage.setItem('nav-filter', newValue); | ||
| //Clear the filter query when filtertext is empty | ||
| if (!newValue) { | ||
| this.$router.replace({ path: this.$route.path, query: {} }); |
There was a problem hiding this comment.
I think we can just push the new route, instead of replacing it.
| this.debouncedUpdateQuery = debounce(newValue => { | ||
| if (newValue) { | ||
| this.$router.push({ | ||
| path: this.$route.path, | ||
| query: { ...this.$route.query, filter: newValue }, | ||
| }); | ||
| } else { | ||
| this.$router.push({ path: this.$route.path, query: {} }); | ||
| } | ||
| }, 2000); // 2-second debounce delay |
There was a problem hiding this comment.
As we wont be using this debouncedUpdateQuery anymore, we can just remove these lines.
| this.debouncedUpdateQuery = debounce(newValue => { | |
| if (newValue) { | |
| this.$router.push({ | |
| path: this.$route.path, | |
| query: { ...this.$route.query, filter: newValue }, | |
| }); | |
| } else { | |
| this.$router.push({ path: this.$route.path, query: {} }); | |
| } | |
| }, 2000); // 2-second debounce delay |
| @@ -51,6 +52,7 @@ | |||
| return { | |||
| filterText: '', | |||
| loaded: false, | |||
| debounceUpdateQuery: null, | |||
There was a problem hiding this comment.
And dont forget to remove this data variable too :)
There was a problem hiding this comment.
Hi @RONAK-AI647! We are not using this state anymore, we can remove it too :)
|
@AlexVelezLl , made the changes. |
Description
In account of the issue#209 , an issue arose where the navigation Table of Content (ToC) was not working properly . This PR talks about this issue only. Our desired behavior in this updation was:
Issue addressed
#213
Addresses #PR# HERE
Before/after screenshots
The first URL
When button is typed in ToC
When button is pressed
FIrst back press
Second back press
Changelog
Steps to test
(optional) Implementation notes
At a high level, how did you implement this?
Does this introduce any tech-debt items?
Testing checklist
Reviewer guidance
Comments
The watch and mount section in docs\common\DocsPageTemplate\SideNav\index.vue is modified.