Notification: show "not reading latest version"

[humitos]

It re-uses the code from NotificationElement. However, I'm not happy with the implementation. It seems that they are pretty much different to make sense to re-use the class.

It seems that what we want here is create a NotificationBaseElement where NotificationExternalElement and NotificationNonLatestElement can extend.

Example

[agjohnson]

This looks great! I only had small notes here

[agjohnson]

However, I'm not happy with the implementation. It seems that they are pretty much different to make sense to re-use the class. It seems that what we want here is create a NotificationBaseElement where NotificationExternalElement and NotificationNonLatestElement can extend.

I don't quite follow here, is there a particular problem you see with the current approach?

Having a single element seems to make sense to me for now. We want both warnings to occupy the same space, and these warnings well never be shown at the same time. If we add a third, unrelated notification, we either want it to occupy the same space (turn the description into a list of messages for example), or I'm assuming we'll make this into a list of notifications instead.

But from a user's perspective, it's odd to require the user/theme author to define every individual notification we define, ie:

<readthedocs-notification-external-version class="..."></readthedocs-notification-external-warning>
<readthedocs-notification-outdated-version class="..."></readthedocs-notification-outdated-warning>
<readthedocs-notification-new-build class="..."></readthedocs-notification-new-build>
<readthedocs-notification-doc-diff class="..."></readthedocs-notification-doc-diff>
<readthedocs-notification-foo class="..."></readthedocs-notification-foo>

If we define different classes for each, then we need separate tags. There might be a better way to abstract the message from the element display though, this could be part of changing the notification to a notification list.

[humitos]

If we define different classes for each, then we need separate tags. There might be a better way to abstract the message from the element display though, this could be part of changing the notification to a notification list.

Yeah, I thought about this and I didn't arrived at a pattern where I was happy with it. I feel the lack of js knowledge here.

My original point about the implementation I've done here is that most of the functions/methods/properties from the class are not required for all the "notifications". For example calculateHighestVersion has nothing to do with the PR notification. this.urls is only used on the PR notification, generating confusions when working with the "non latest version notification", etc.

I think it makes sense to use the same <readthedocs-notification> tag. Is is possible to have two different isolated classes with their own functions but sharing the same HTML tag? Would that make sense? 🤔

[humitos]

On a different topic, it seems that our shadow DOM does not respect the exact same style on different sites:

Sphinx

Material for MkDocs

Do you have an idea what it could be?

[agjohnson]

Yeah, I thought about this and I didn't arrived at a pattern where I was happy with it.

Same, but I think we can develop this over time too and don't need to nail this implementation just yet. For basic/default usage, I think what we have now works well, but customization is where we will want to start building up this abstraction pattern more.

This is where I probably want to see what theme maintainers want to do with our data, and make this implementation flexible enough to fit those needs. For example, I see maintainers not wanting to use this notification element at all, and instead wanting the underlying information (version is outdated/version is a pull request) directly so they can adjust independent visual elements.

I can put up an issue with a few different directions from the HTML/JS side of things, maybe that is a good place to get some feedback to start.

most of the functions/methods/properties from the class are not required for all the "notifications"

Better naming could help here. Overall, the current design isn't bad though. When we figure out what the additional layer of abstraction is, this will feel cleaner.

is possible to have two different isolated classes with their own functions but sharing the same HTML tag?

It is not, and this will throw and exception actually.

Do you have an idea what it could be?

This looks like it's just font size, which is expected. The rem unit is still a relative unit in CSS ¹, but it is only relative to the root element font size. The em unit is also relative, but to the parent element and every parent element up the chain.

This behavior is what I'd expect though, and is generally more correct than dictating a different font size than the root element. To avoid this, we could use px or another absolute unit, which wouldn't be awful given the font-size is a CSS variable. But, generally, respecting the rem is going to be the best fit visually.

Footnotes

1. https://developer.mozilla.org/en-US/docs/Learn/CSS/Building_blocks/Values_and_units#lengths ↩

[agjohnson]

I've dropped some example patterns in #51 for now. But I'd say lets stew on that a bit, we don't need to get too fiddly just yet.

[humitos]

I updated the PR with the feedback received and I will merge it. I'm sure there will be more work to be done here, but we can always open more PRs 😉