You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
It seems better to split the NotificationAddon in two (or three) different addons: NonLatestVersionWarning, NonStableVersionWarning and ExternalVersionWarning (or simlar names).
There are different reasons to do this. The main reason to me (among code isolation per addon), is the ability to enable and disable each of them based on data validation. Currently, if there is only one field missing, all the notifications are disabled but maybe that field is only used for the "latest version warning".
I think all those addons should share some of the logic by extending the Notification class but they should be different addons. I'm not sure if they should be different WebComponent as well or not (e.g. readthedocs-notification-latest-version-warning or just readthedocs-notification but with different initial setup)
The text was updated successfully, but these errors were encountered:
Yes. I understand this is how it currently works, right? We have all the logic mixed into the same class and depending on some values we decide what path to follow. IMO, this is more confusing than helpful.
I think we could share the common login between these addons in a NotificationBase class or similar and implement only the different chunk of login in the addon specific class.
It seems better to split the
NotificationAddon
in two (or three) different addons:NonLatestVersionWarning
,NonStableVersionWarning
andExternalVersionWarning
(or simlar names).There are different reasons to do this. The main reason to me (among code isolation per addon), is the ability to enable and disable each of them based on data validation. Currently, if there is only one field missing, all the notifications are disabled but maybe that field is only used for the "latest version warning".
I think all those addons should share some of the logic by extending the
Notification
class but they should be different addons. I'm not sure if they should be different WebComponent as well or not (e.g.readthedocs-notification-latest-version-warning
or justreadthedocs-notification
but with different initial setup)The text was updated successfully, but these errors were encountered: