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

Notification: render it inline using docTool.getRootSelector() #552

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

humitos
Copy link
Member

@humitos humitos commented Feb 20, 2025

Instead of rendering the notification floating at the top right, we use the heuristic from DocumentationTool class to get the root selector and we prepend the notification WebComponent there.

Example

Small example showing how it behaves with some of the known generators.

Peek.2025-02-20.12-25.webm

ToDo

  • Tweaks selectors -- we may want a different selector than the root one, specific for this use case. I'm not sure. I started writing this code, but It seems it's not needed for now.
  • Add style rules to some doctools to render accordingly.
  • Write some tests.
  • Feedback from the team.

Closes #550

Instead of rendering the notification floating at the top right, we use the
heuristic from `DocumentationTool` class to get the root selector and we prepend
the notification WebComponent there.

Closes #550
@humitos
Copy link
Member Author

humitos commented Feb 24, 2025

One of the downsides this pattern has is the content shift when the notification appears:

Peek.2025-02-24.16-49.webm

Even with that, I think it's better than the current behavior that hides/blocks content. The Sphinx extension I built some time ago behaves in the same way: https://github.com/humitos/sphinx-version-warning

I think it's acceptable.

Take into account that the "version selector" below the title our own theme builds requires the same extra time to load.

@humitos humitos marked this pull request as ready for review February 24, 2025 16:37
@humitos humitos requested a review from a team as a code owner February 24, 2025 16:37
@humitos humitos requested a review from agjohnson February 24, 2025 16:37
@agjohnson
Copy link
Contributor

Yeah CLS is going to be a common problem with all of this given it is injected content.

The main thing I'm still concerned with here is styling/customization, this is a more obvious alteration of user/project content than floating content. I think it would be helpful to start with something we can test without changing all projects at once, and we should aim to implement better customization before making this the default. The notification looks a bit out of place in most of the examples due to slightly different colors/borders/corners and where it lands on the page (above/below the main page title).

This is looking close and I'd agree it's getting towards a better UX than overlaying important user content.

@humitos
Copy link
Member Author

humitos commented Feb 25, 2025

The main thing I'm still concerned with here is styling/customization, this is a more obvious alteration of user/project content than floating content.

Would you be 👍🏼 if we inject different HTML chunk based on the documentation tool? That way, we can use the same native notification HTML structure the tool uses.

Sphinx example:

<div class="admonition note">
  <p class="admonition-title">Note</p>
  <p>Godot's documentation is available in various languages and versions. Expand the "Read the Docs" panel at the bottom of the sidebar to see the list.</p>
</div>

and it will render as:

Screenshot_2025-02-25_11-02-51

Just an example for look and feel, the text should be the current copy we have in our own notification.

In any case, even if we have specific notification for each of the documentation tools, we still need a generic one to render when we don't know the documentation tool. I think it makes sense to focus on this generic one, first.

@humitos
Copy link
Member Author

humitos commented Feb 25, 2025

I think it would be helpful to start with something we can test without changing all projects at once, and we should aim to implement better customization before making this the default.

What type of customization are you thinking about here?

The notification looks a bit out of place in most of the examples due to slightly different colors/borders/corners

What changes we can do to the generic style to behave better?

and where it lands on the page (above/below the main page title).

I started writing the code to have a specific selector per documentation tool that I can continue working on to solve this issue.

@humitos humitos requested a review from ericholscher February 25, 2025 10:08
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.

Notifications: use heiristic to add the notification inline
2 participants