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

[Navigate Results Modern][Added] Revamped version #768

Merged
merged 14 commits into from
Jan 21, 2025

Conversation

nguyen-v
Copy link

@nguyen-v nguyen-v commented Jan 10, 2025

This PR adds a new output navigate_results_modern, which is a revamped version of the navigate_results output. Because of the many changes, I've decided to make it a new output. It has many features, such as:

  • a modern interface, with elements that automatically fit/scale with the layout
  • dark and light modes
  • an expandable side navigation panels with expandable folders
  • a search bar for outputs
  • the possibility to render markdown files
    The configuration is very similar to the navigate_results output:
kibot:
  version: 1

outputs:
  type: navigate_results_modern
  options:
    link_from_root: 'index.html'
    logo: 'Logos/amulet_logo_small_light.png'
    logo_url: 'https://github.com/nguyen-v/amulet_controller_kibot'
    nav_bar: true
    render_markdown: true
    title: 'Title test'

When not running on a server, it works best on edge/chrome (because of issues with firefox's localStorage). When running it on a local server, any browser is fine

You can see an example in this video (download it to see it):
https://github.com/user-attachments/assets/83ef5260-b9d8-4136-9462-a7eb2382f306

@set-soft
Copy link
Member

Hi @nguyen-v !

This navigate results look really nice. I have some concerns:

  1. It repeats too much code, so I think it should be merged with the current version. For this we could move some rendering methods to a class dedicated to it. Then we could have 2 classes and instantiate an object according to some setting that selects the aspect. I saw important differences in generate_cat_page_for, generate_outputs, generate_navbar_one (generate_sidenav_one), generate_navbar (generate_sidenav) and generate_top_menu. They should be part of the class selecting the aspect. Each "aspect" class could be in a separated source code, to keep its CSS/JS close. The Navigate_ResultsOptions can be the same, just mentioning that header doesn't apply to the alternative version. We could also "backport" the markdown stuff to the current version.
  2. It adds an external dependency, which isn't a good idea, the users should be able to navigate the results without internet connection. If https://cdn.jsdelivr.net/npm/markdown-it/dist/markdown-it.min.js is atomic we should include a copy and use it. BTW: it should be included only when the markdown option is enabled, no?
  3. add_to_tree has some modifications to detect ., should be documented. Making it will work with both styles.

Then I have some aspect preferences

  1. The lack of images in the main categories. This should be an option. Images there are for fast navigation.
  2. The KiBot version at the bottom is missing
  3. The navigation arrows looks like a copy of the browser arrows. The one added in the current version is to go back in the hierarchy. This is more important.
  4. The name "modern" is confusing other, more representative, name should be used
  5. The logo position and size should be like in the current version, not forced to be as a character. The user can specify a logo and hence choose the size, I don't like forcing a size

@nguyen-v
Copy link
Author

nguyen-v commented Jan 14, 2025

  1. It repeats too much code, so I think it should be merged with the current version. For this we could move some rendering methods to a class dedicated to it. Then we could have 2 classes and instantiate an object according to some setting that selects the aspect. I saw important differences in generate_cat_page_for, generate_outputs, generate_navbar_one (generate_sidenav_one), generate_navbar (generate_sidenav) and generate_top_menu. They should be part of the class selecting the aspect. Each "aspect" class could be in a separated source code, to keep its CSS/JS close. The Navigate_ResultsOptions can be the same, just mentioning that header doesn't apply to the alternative version. We could also "backport" the markdown stuff to the current version.

Agree that it should be refractored.

  1. It adds an external dependency, which isn't a good idea, the users should be able to navigate the results without internet connection. If cdn.jsdelivr.net/npm/markdown-it/dist/markdown-it.min.js is atomic we should include a copy and use it. BTW: it should be included only when the markdown option is enabled, no?

I've tried adding the .js file in resources and copying it into the output dir when generating the HTML and it's working great

  1. add_to_tree has some modifications to detect ., should be documented. Making it will work with both styles.

Will add some documentation

  1. The lack of images in the main categories. This should be an option. Images there are for fast navigation.

I will see what I can do, but I personally felt like it was getting confusing between the categories and output images, making it harder to distinguish which is which

  1. The KiBot version at the bottom is missing

Will add an option for this

  1. The navigation arrows looks like a copy of the browser arrows. The one added in the current version is to go back in the hierarchy. This is more important.

Personally I find it more intuitive this way, as it's similar to how most file explorers are. Maybe I can make the navigation path clickable to go back in the hierarchy? Windows explorer has an additional arrow (pointing up) to go back in the hierarchy

  1. The name "modern" is confusing other, more representative, name should be used

Mm can't think of a good name, any suggestions?

  1. The logo position and size should be like in the current version, not forced to be as a character. The user can specify a logo and hence choose the size, I don't like forcing a size

Got it

@set-soft
Copy link
Member

  1. It repeats too much code, so I think it should be merged with the current version. For this we could move some rendering methods to a class dedicated to it. Then we could have 2 classes and instantiate an object according to some setting that selects the aspect. I saw important differences in generate_cat_page_for, generate_outputs, generate_navbar_one (generate_sidenav_one), generate_navbar (generate_sidenav) and generate_top_menu. They should be part of the class selecting the aspect. Each "aspect" class could be in a separated source code, to keep its CSS/JS close. The Navigate_ResultsOptions can be the same, just mentioning that header doesn't apply to the alternative version. We could also "backport" the markdown stuff to the current version.

Agree that it should be refractored.

I see two paths:

  1. Make it a "style", one class and an option selects it. A secondary class handles the aspect differences
  2. Make something like the "any" classes, a base class that solves the common stuff and 2 classes that fill the gaps
  1. The navigation arrows looks like a copy of the browser arrows. The one added in the current version is to go back in the hierarchy. This is more important.

Personally I find it more intuitive this way, as it's similar to how most file explorers are. Maybe I can make the navigation path clickable to go back in the hierarchy? Windows explorer has an additional arrow (pointing up) to go back in the hierarchy

I think we should have an arrow at the left of the "Home", to make it coherent with the other style.
We can keep the current back/forward stuff, but again: they looks like a repetition of the browser arrows.
About clicking the path: This could be used to jump to an arbitrary ancestor

  1. The name "modern" is confusing other, more representative, name should be used

Mm can't think of a good name, any suggestions?

Depending on the implementation:

  1. For a style it could be something like "rounded boxes"
  2. For separated classes it should be shorter, because we have an already long name, like "navigate_results_rb"

@nguyen-v
Copy link
Author

nguyen-v commented Jan 18, 2025

I've implemented the following:

All styles:

  • Refractoring: now the classes derive from Any_Navigate_ResultsOptions
  • Back-ported markdown rendering to the current navigate_results. Now doesn't need an internet connection.
  • Added and documented the option to handle outputs at the root of the tree for all navigate_results styles
  • Added an option to display/hide the kibot version
  • Added an option to display/hide the category images
  • Added an option to force the height of the logo. This is useful to get consistent results across different logos.

Rounded box style:

  • Made the path clickable to access any ancestor
  • Added up arrow to go up a category
  • Moved the home button next to the up button
  • Some minor adjustments for the wrapping of the path

image

@set-soft set-soft merged commit d7ba40b into INTI-CMNB:dev Jan 21, 2025
17 checks passed
@set-soft
Copy link
Member

Hi @nguyen-v !
I merged the code thanks.
But I think it needs more adjusts.
The first doubt is about the change from BasicOptions to VariantOptions. This output isn't related to variants, is this to get the variant name? I don't think this is really needed.

@nguyen-v
Copy link
Author

is this to get the variant name? I don't think this is really needed.

Right, I forgot to change it back, I was experimenting with variant but in the end the way it gets the variant name doesn't need VariantOptions

Also it seems I forgot to delete a garbage file from WSL2 (kibot/resources/navigate_results/markdown-it.min.js:Zone.Identifier, sorry about that

@nguyen-v
Copy link
Author

nguyen-v commented Jan 25, 2025

For some reason, I get Missing resource directory 'navigate_results', but only when using the docker. There's no error when running locally. Is there something missing in the docker images? Maybe it has to do with the MANIFEST.in I'm not sure

set-soft added a commit that referenced this pull request Jan 27, 2025
- To Python package
- To Debian package

See #768
@set-soft
Copy link
Member

For some reason, I get Missing resource directory 'navigate_results', but only when using the docker.

The above patch should solve it

There's no error when running locally. Is there something missing in the docker images? Maybe it has to do with the MANIFEST.in I'm not sure

The manifest is for PyPi, so it was missing.
For Debian is debian/install
Both now contains the missing JS

@nguyen-v
Copy link
Author

For some reason, I get Missing resource directory 'navigate_results', but only when using the docker.

The above patch should solve it

There's no error when running locally. Is there something missing in the docker images? Maybe it has to do with the MANIFEST.in I'm not sure

The manifest is for PyPi, so it was missing.
For Debian is debian/install
Both now contains the missing JS

Thanks!

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.

2 participants