-
Notifications
You must be signed in to change notification settings - Fork 19
Notification: render it inline using docTool.getRootSelector()
#552
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
Conversation
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
One of the downsides this pattern has is the content shift when the notification appears: Peek.2025-02-24.16-49.webmEven 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. |
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. |
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:
and it will render as: 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. |
What type of customization are you thinking about here?
What changes we can do to the generic style to behave better?
I started writing the code to have a specific selector per documentation tool that I can continue working on to solve this issue. |
I am not aiming to focus on specific changes to our notification to make it look better across more tools. I'm only pointing out that it looks unpolished on most tools and therefore pushing out a change to inject the notification inline, without giving users control of this styling or customization to the styling, is a bad move on our part.
I think this is closer to a better path, we aren't responsible for styles in this case. This will require us to use a "light DOM" element however -- these do not use the shadow DOM. This is the piece required to allow styles for However, that also feels like extra work for this stage. I still feel what I described above: we shouldn't change this for all projects yet and we need to provide more customization if we're altering user content directly. |
Yes. I "created an issue to create issues" for the required work. I'm linking it here, so we have all of these issues backlinked somewhere. I'm closing this one for now since it doesn't make too much sense to move forward with this approach without further discussion and experiment with the new UI. |
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.Closes #550