OSC / ondemand

Supercomputing. Seamlessly. Open, Interactive HPC Via the Web
https://openondemand.org/
MIT License
281 stars 104 forks source link

Redesign how we add Menus to the NavBar #2154

Open gerald-byrket opened 2 years ago

gerald-byrket commented 2 years ago

It is not scalable to have to create a new app for every custom NavBar. We need to make this better.

┆Issue is synchronized with this Asana task by Unito

johrstrom commented 2 years ago

@adaybujeda IDK where else to have this conversation but we can have it here with all your PRs

To wit: I'm wondering if we can't figure a scheme where the nav bar's structure is defined entirely through YAML.

Here's what one such config may look like. We can take the logic in TokenMatcher.

nav_bar:
  - files  # single string means group_by(:category)
  - title: "custom nav entry"
    apps: 
      - sys/rstudio
      - sys/jupyter
      - type: sys
        category: 'minimal'
    group_by: subcategory
  - title: "just links"
     apps:
      - link: 'some.other.site'
        icon: 'clock'

It's a much larger change, yes, but I get the feeling we're adding new config fields for this and that when we really want a way to do just about anything.

abujeda commented 2 years ago

@johrstrom this was my next item (just created #2244). Initial idea was a way of adding custom items to the existing navigation, not to define the full navigation. But the implementation might be much simpler if it is a full navigation definition in YAML.

johrstrom commented 2 years ago

But the implementation might be much simpler if it is a full navigation definition in YAML.

Yea from a config standpoint it's much easier too. I'd rather given folks 1 complex object to define than N that are related, but unique in some way. There's a documentation & just mental model burden here too.

We want to redefine the entire navbar, so I'm thinking we should just make that as straightforward as possible, both for us and admins.

abujeda commented 2 years ago

First idea to make a full navigation. I thought that making the nav configuration more explicit might be easier to understand.

At a high level, something like this:

nav_bar:
  - menu:
      links:
        - link_header
        - link
        - link
        - link_header
        - link
        - hline

  - menu
  - link
  - menu
  - menu
  - link

The order in the config will be the order in the navigation. No automatic grouping, this is defined in the config with link headers and horizontal lines Menus and links text and other metadata defined in the configuration.

A link could be:

This will make it very verbose as you need to define each item.

abujeda commented 2 years ago

Adding to the comment above, this is a prototype for the full navigation definition. These are some ideas of what items can be added. I am still thinking of other types to help with the definition.

I hope that the configuration is self-explanatory. Image of the result added below.

Configuration:

nav_bar:
  - type: menu
    title: "Help"
    links:
      - type: divider
        title: "Help Docs"
      - type: link
        title: "Help Docs"
        icon: fa://desktop
        url: "/test"
        new_tab: false
      - type: divider
      - type: app
        title: "SAS Link"
        token: "sys/sas"
  - type: menu_link
    title: "Documentation"
    url: "/sid2"
    new_tab: true
  - type: menu
    title: "Docs"
    links:
      - type: link
        title: "Page Docs"
        icon: fa://desktop
        url: "/test"
        new_tab: false
  - type: menu_app
    title: "SAS Menu"
    token: "sys/sas"
  - type: menu_link
    title: "Wiki"
    icon: fa://desktop
    url: "/sid2"
    new_tab: true
  - type: menu_link
    title: "Left Link"
    icon: fa://desktop
    url: "/test"
    new_tab: false
  - type: menu_link
    title: "About"
    url: "https://www.iq.harvard.edu/research-computing#AboutSidNG"
    new_tab: false
Screenshot 2022-08-19 at 14 45 27
johrstrom commented 2 years ago

Let's try to avoid type if we can. We should be able to make some inferences about what's supplied. I think dividers can be done by adding subcategories to links (the title is the category)?

  # has a url so it's a link
  - title: "Documentation"
    url: "/sid2"
    new_tab: true

  # has apps so it's an app group, but only 1 app so it's just a single link
  - apps:
      - sys/jupyter

  # has multiple apps - so looks like a regular app group 
  - apps:
     - sys/rstudio
     - sys/vmd
abujeda commented 2 years ago

I have simplified the config by making types optional and inferring them on the back end. I think types are important as they simplify the implementation and allow the addition of new features without affecting the existing ones.

As well, I think that having explicit dividers makes a easier to configure. More so when the same app might be group differently in different profiles.

In the backend, there are 5 types:

nav_bar:
  - title: "Help"
    links:
      - title: "Help Docs"
      - title: "how to docs"
        icon: fa://desktop
        url: "/docs"
        new_tab: false
      - title: "submit a help ticket"
        icon: fa://question-circle
        url: "/tickets"
        new_tab: false
      - title:
      - title: "SAS Link"
        token: "sys/sas"
  - title: "Documentation"
    url: "/sid2"
    new_tab: true
  - title: "Apps Menu"
    links:
      - title: "Main App"
      - token: "sys/sas"
      - title: "Core Apps"
      - tokens: ["sys/bc_desktop", "sys/sas", "sys/Rstudio/*"]
  - title: "SAS Menu"
    token: "sys/sas"
  - title: "Wiki"
    icon: fa://desktop
    url: "/sid2"
    new_tab: true
  - title: "Left Link"
    icon: fa://desktop
    url: "/test"
    new_tab: false
  - title: "About"
    url: "https://www.iq.harvard.edu/research-computing#AboutSidNG"
    new_tab: false

This renders this nav:

Screenshot 2022-08-22 at 12 34 19 Screenshot 2022-08-22 at 12 34 34
johrstrom commented 2 years ago

NavMenu NavLink NavApp NavAppMatcher NavDivider

Without seeing the code I can only say, be careful writing POJOs in ruby. POROs (plain old ruby objects) seem to be an anti pattern when you can use Hashes or factories + delegators or some 3rd pattern I'm sure exists.

abujeda commented 2 years ago

Yes, I was going in a very POJO oriented way. I have update the prototype based on the comment above.

I created a PR to get your views on the direction taken for the prototype implementation: https://github.com/OSC/ondemand/pull/2260

abujeda commented 2 years ago

Added new type to support adding links to the interactive sessions and the all apps. This new type is a template.

nav_bar:
  - title: "Help"
    links:
      - title: "Help Docs"
      - title: "how to docs"
        icon: fa://desktop
        url: "/docs"
        new_tab: false
      - title: "submit a help ticket"
        icon: fa://question-circle
        url: "/tickets"
        new_tab: false
      - title:
      - title: "SAS Link"
        token: "sys/sas"
      - tokens: ["sys/Rstudio/*"]
  - title: "Documentation"
    url: "/sid2"
    new_tab: true
  - title: "Apps Menu"
    links:
      - title: "Main App"
      - token: "sys/shell"
      - title: "Sas Main"
        token: "sys/sas"
      - title: "SubApp"
        tokens: ["sys/Rstudio/gpu"]
      - title: "Core Apps"
      - tokens: ["sys/Rstudio/*"]
  - title: "SAS Menu"
    token: "sys/sas"
  - title: "Wiki"
    icon: fa://desktop
    url: "/sid2"
    new_tab: true
  - title: "Left Link"
    icon: fa://desktop
    url: "/test"
    new_tab: false
  - title: "About"
    url: "https://www.iq.harvard.edu/research-computing#AboutSidNG"
    new_tab: false
  - template: "sessions"
  - template: "all_apps"
Screenshot 2022-08-23 at 15 29 53
abujeda commented 2 years ago

We could add support for translations by using something like this in configuration for titles: - title: "i18n://dashboard.nav_all_apps"

johrstrom commented 2 years ago

It seems like - template: "all_apps" should already respond to the i18n key associated (dashboard.nav_all_apps) with it right?

abujeda commented 2 years ago

Yes, that is right. I just thought that it might be useful to support translations for the custom titles defined in the configuration. Not sure how localizations are used in general within the different institutions, but as the navigation is a very visible part of the site, I thought it might be needed.