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

Tweak site navigation design #1222

Merged
merged 2 commits into from
Jun 7, 2020
Merged

Conversation

ang-zeyu
Copy link
Contributor

@ang-zeyu ang-zeyu commented May 4, 2020

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Enhancement to an existing feature

Requires #1221 (simplifies / removes the recursive process so that the changes here make more sense)

What is the rationale for this request?

  • make the site nav shinier

What changes did you make? (Give an overview)

  • tweak the sitenav dom structure, classes, styles to:
    • provide a highlight background color on hover
    • automatically style distinct levels of headings (up to 3 levels of indentation - ie 4 total styles)

sitenav

Is there anything you'd like reviewers to focus on?

  • the new design

Testing instructions:

  • npm run test should pass

Proposed commit message: (wrap lines at 72 characters)
Redesign site navigation sidebar

@ang-zeyu
Copy link
Contributor Author

ang-zeyu commented May 4, 2020

@damithc could I get your opinion on the tweaks here?

*the pr currently makes clicking a dropdown that is also a link expand/collapse the dropdown (will be fixed)

@damithc
Copy link
Contributor

damithc commented May 4, 2020

@damithc could I get your opinion on the tweaks here?

Sounds good @ang-zeyu 👍

@ang-zeyu ang-zeyu changed the title Tweak site navigation design [WIP] Tweak site navigation design May 4, 2020
@ang-zeyu ang-zeyu force-pushed the site-nav-tweaks branch 2 times, most recently from e5813f5 to 4032423 Compare May 5, 2020 03:25
@ang-zeyu ang-zeyu changed the title [WIP] Tweak site navigation design Tweak site navigation design May 5, 2020
@ang-zeyu ang-zeyu force-pushed the site-nav-tweaks branch 2 times, most recently from b091482 to b60031b Compare May 5, 2020 08:16
@ang-zeyu ang-zeyu requested a review from yamgent May 19, 2020 15:48
@ang-zeyu ang-zeyu added this to the v2.15.0 milestone Jun 7, 2020
@ang-zeyu ang-zeyu merged commit cd4cbaf into MarkBind:master Jun 7, 2020
@damithc
Copy link
Contributor

damithc commented Jun 11, 2020

@ang-zeyu We seem to have lost some of the flexibility we had before though. For example, it was possible to use normal HTML to tweak the appearance of the siteNav e.g., use <hr> <br> etc. Is it possible to add them back? Would that complicate the code too much?

@ang-zeyu
Copy link
Contributor Author

not easily, the issue is that previously, the <hr/br> was being inserted into the <li> item. It wasn't an issue then as there was no box highlighting, only a link.

With the "box" this becomes an issue, as the whole <li> is highlighted, and we don't want the extra content to extend the highlight box.

There isn't a way afaik in markdown to insert content between the <li>'s (the * ...'s - it just gets chucked back into the <li>) either, so it's not possible to differentiate content meant (from author standpoint) to be "outside the box" from inside. (unless you manually write <ul><li>...</li><hr><li>...</li></ul>)

I could hardcode <hr/br>'s to be moved outside the "box", but its hardly an ideal solution since then we'd have custom content like searchbars, videos, ads maybe even 😬.

Instead, how about playing to the markdown spec and using another list item to insert custom content?

* ... link ...
* <hr>
* ... link ...

This works currently, but an extra "box" with highlight is created for the <hr> item as well.

  1. In this case I could easily blacklist list items that don't have nested <ul>'s or <a>'s from getting the highlight.
  2. Or necessitate the user to use markdown-it-attrs to do
    * <hr> {.no-highlight} (we should deprecate :expanded: to markdown-it-attrs at some point too)

@damithc
Copy link
Contributor

damithc commented Jun 11, 2020

This works currently, but an extra "box" with highlight is created for the <hr> item as well.

  1. In this case I could easily blacklist list items that don't have nested <ul>'s or <a>'s from getting the highlight.
  2. Or necessitate the user to use markdown-it-attrs to do
    * <hr> {.no-highlight} (we should deprecate :expanded: to markdown-it-attrs at some point too)

We can try option 1. In that case, need to document that 'trick' in the user guide as it will not be obvious. Can create an issue and get to it later as it is not urgent (because old sites using rouge syntax is going to break anyway, even with that solution).

@ang-zeyu
Copy link
Contributor Author

Can create an issue and get to it later as it is not urgent (because old sites using rouge syntax is going to break anyway, even with that solution).

Should be a pretty minor change, will lump it in with #1241 as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants