Skip to content
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

[cs2103 website] Some site-nav items are not highlighted when open #1615

Open
damithc opened this issue Jun 29, 2021 · 8 comments
Open

[cs2103 website] Some site-nav items are not highlighted when open #1615

damithc opened this issue Jun 29, 2021 · 8 comments
Labels
c.Bug 🐛 p.High To be done in the next few releases s.ToInvestigate

Comments

@damithc
Copy link
Contributor

damithc commented Jun 29, 2021

Deployed site: https://nus-cs2103-ay2021s2.github.io/website/admin/index.html

Clicking on this site-nav item will auto-expand the parent menu and highlight the item in blue after the target page is loaded, as expected.
image

Problem: If I click on this item, the correct item is highlighted in blue but the parent menu is not auto-expanded after the target page is loaded.
image

The target page looks like this:
image

If I expand the parent menu manually, the correct menu item is actually highlighted in blue.
image

Steps to reproduce locally:

The relevant site-nav code is here https://raw.githubusercontent.com/nus-cs2103-AY2021S2/website/master/_markbind/layouts/sitenav-admin.md

This is not urgent or critical but hope someone can help me locate the problem -- could be a problem in my code rather than a MarkBind bug.

@ang-zeyu
Copy link
Contributor

Also occurs in week 7.

Try removing parts of the page content temporarily and see what happens. Could be caused by something else

@jonahtanjz
Copy link
Contributor

Did some investigation and I believe it's something related to hydration issue after introducing SSR. Somehow after detecting a hydration issue, some of the states get reset, which explains why this issue only affects certain pages. I'm not too familiar with this perhaps @wxwxwxwx9 can take a look at this as well? Similarly, in #1616, the added class dropdown-current gets removed after bailing hydration.

@wxwxwxwx9
Copy link
Contributor

wxwxwxwx9 commented Jul 3, 2021

Hmm @jonahtanjz can I check how did you replicate the hydration issue and what is it about? :O

I was able to replicate the bug as described in this issue but couldn't replicate the hydration issue on my side.

@jonahtanjz
Copy link
Contributor

jonahtanjz commented Jul 3, 2021

Sorry @wxwxwxwx9 hydration issue is probably not the cause here. Was looking at the wrong page 😓 It occurs on pages without hydration issue as well.

@damithc
Copy link
Contributor Author

damithc commented Dec 18, 2021

Based on se-edu/se-book#112, this may be caused by popover elements in the page.

@damithc
Copy link
Contributor Author

damithc commented Dec 26, 2021

Based on se-edu/se-book#112, this may be caused by popover elements in the page.

I experimented with admin/tp-constraints.md file and found that the problem goes away if I remove all <tooltip> elements from the page.

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Dec 27, 2021

Confirmed this while working on #1698 as well.

Did some investigation and I believe it's something related to hydration issue after introducing SSR. Somehow after detecting a hydration issue, some of the states get reset, which explains why this issue only affects certain pages. I'm not too familiar with this perhaps @wxwxwxwx9 can take a look at this as well? Similarly, in #1616, the added class dropdown-current gets removed after bailing hydration.

A git bisect reveals the causing PR is indeed the SSR one #1534.

Edit: The second (deleted) part is probably a bad guess

https://ssr.vuejs.org/guide/universal.html#custom-directives
https://ssr.vuejs.org/api/#directives

While bootstrap vue's docs don't seem to mention their directives being incompatible with SSR (I mistakenly assumed so), this could be the case and the cause here.

We could try changing all of our popover / tooltip implementations to use bootstrap vue's components instead.

@ang-zeyu
Copy link
Contributor

Simple reproduction:

  • go to a using components page with a popover / tooltip used
  • Remove :expanded: from layouts/userGuide.md (line 16 or so)

@ang-zeyu ang-zeyu added p.High To be done in the next few releases and removed p.Medium labels Jan 30, 2022
jonahtanjz pushed a commit that referenced this issue Feb 13, 2022
Currently, popovers and tooltips are implemented using bootstrap-vue 
directives. 

Such an implementation is not reactive which limits the content that 
can be used in a popover/tooltip. Reactivity is important for future 
enhancements like supporting the src attribute (#59). Furthermore, it 
is incompatible with SSR, resulting in hydration problems that have 
arisen in #1615.

Using Vue components for tooltips and popovers, and Portals for 
triggers, allows us to avoid the issues with reactivity and SSR. 

Let's use components instead of bootstrap-vue directives to implement 
popovers and tooltips. This gives better support of reactivity while 
avoiding SSR issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug 🐛 p.High To be done in the next few releases s.ToInvestigate
Projects
None yet
Development

No branches or pull requests

4 participants