Skip to content

NG: Theme modernization, last mile#506

Merged
amotl merged 47 commits intoamo/basic-ngfrom
matthias/theme-improvements
Jul 18, 2024
Merged

NG: Theme modernization, last mile#506
amotl merged 47 commits intoamo/basic-ngfrom
matthias/theme-improvements

Conversation

@amotl
Copy link
Copy Markdown
Member

@amotl amotl commented Jul 8, 2024

@amotl
Copy link
Copy Markdown
Member Author

amotl commented Jul 9, 2024

Thank you very much for adding so much progress to this. Hereby, I am starting a small collection of backlog items. Some of them might be unrelated to the modernization, some indeed are.

@msbt
Copy link
Copy Markdown
Contributor

msbt commented Jul 9, 2024

fixed bigger quote text via 786fd2a (apparently that also happened on the current docs, but I couldn't find an example)

@msbt
Copy link
Copy Markdown
Contributor

msbt commented Jul 15, 2024

@amotl the ToC error is "fixed" with 08586b7

Pages with code snippets or other content that's wider than the screen don't screw up the layout anymore, had to give up fluid containers for that, but we'll survive without ;)

I did however find a strange 404 which tries to load a css asset from /_static/ (without a filename), unsure where that comes from or what it is.

Other than that: Algolia instantsearch is included now, but needs some more love to get the URL right (i.e. it points to
https://crate-docs-theme--506.org.readthedocs.build/search.html?q=joins instead of
https://crate-docs-theme--506.org.readthedocs.build/en/506/search.html?q=joins

Honorable mentions: left sidebar has a grey background now and should always be boxed and not overflow at the bottom. With that fixed, I gave the menu items 2px more padding for easier navigation and better UX. Search button is square again to fit in with the other elements. Also added and finetuned a few breakpoints for when the sidebars and header disappear.

@amotl
Copy link
Copy Markdown
Member Author

amotl commented Jul 16, 2024

Hi. I love the appearance as seen on the preview now. Thanks a stack for working on this and bringing it to this level. 🌻

While the overall appearance is not revolutionary different than before, everyone who knows reasonably well about the ingredients of the new theme will very much appreciate it, and we certainly do!

One final things comes to mind: What about slotting in the SEARCH button into a different location instead? When possible, let's also hear about voices from others on this and other details. Thanks!

/cc @seut, @matriv, @matkuliak, @proddata, @ckurze, @geragray

NB: 1_572 lines of codes removed. Getting rid of old-fashioned CSS which added drag to maintenance? Great job!

@proddata
Copy link
Copy Markdown

Looks great 👍
a few suggestions:
image

@msbt
Copy link
Copy Markdown
Contributor

msbt commented Jul 16, 2024

@proddata I had the search in the left sidebar once already, but this means on mobile it will only be visible when the sidebar is open (or we duplicate the field and show it somewhere else when it disappears).

I was also playing around with the position of the version chooser and it didn't look great when it wasn't aligned with the top nav. However, if we lose it, that shouldn't be a problem anymore. Should we keep Login/Get CrateDB or drop them as well?

Btw, can you do a hard refresh? No idea where some of those background colors are coming from in your screenshot, I don't see that bar on the right ;)

@proddata
Copy link
Copy Markdown

@proddata I had the search in the left sidebar once already, but this means on mobile it will only be visible when the sidebar is open (or we duplicate the field and show it somewhere else when it disappears).

Considering that searching is going away from the page anyway I would consider it fine.
I think the sidebar(s) have a much bigger issue on mobile devices in the sense that the font ins tiny ;)

Should we keep Login/Get CrateDB or drop them as well?

We might want to keep "Login" and "Get CrateDB" and add a button back to the homepage?

Btw, can you do a hard refresh? No idea where some of those background colors are coming from in your screenshot, I don't see that bar on the right ;)

Are you using an external mouse or a device with just a touchpad?
It is a scrollbar. If that is not shown, the version picker is less of a problem

image

@msbt

This comment was marked as resolved.

@amotl
Copy link
Copy Markdown
Member Author

amotl commented Jul 18, 2024

One final things comes to mind: What about slotting in the SEARCH button into a different location instead?

Thank you very much for your recent changes. Perfect! 🌻

@proddata

This comment was marked as resolved.

@proddata
Copy link
Copy Markdown

Looks good now 👍

@amotl amotl force-pushed the matthias/theme-improvements branch from e06e187 to 7576f47 Compare July 18, 2024 17:07
@amotl amotl force-pushed the matthias/theme-improvements branch from 7576f47 to 4253506 Compare July 18, 2024 17:14
@amotl amotl requested a review from msbt July 18, 2024 17:17
@amotl amotl assigned proddata and matkuliak and unassigned proddata and matkuliak Jul 18, 2024
@amotl amotl requested review from matkuliak and proddata July 18, 2024 17:17
@amotl amotl marked this pull request as ready for review July 18, 2024 17:17
Copy link
Copy Markdown
Member Author

@amotl amotl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this patch, @msbt. We will integrate the improvements into GH-390, and run a devX release, in order to be able to slap the new theme on a few downstream projects in preview mode.

@amotl amotl merged commit 35e7965 into amo/basic-ng Jul 18, 2024
@amotl amotl deleted the matthias/theme-improvements branch July 18, 2024 17:22
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.

4 participants