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

ui: When peerings are enabled add a HTML.class to tell the UI CSS #14520

Closed
wants to merge 1 commit into from

Conversation

johncowen
Copy link
Contributor

Description

Whilst working on some upgrades with @LevelbossMike we noticed that a HTML class was missing for when peerings was enabled. This adds that class so it is consistent with other larger feature flagged features and available if/when needed.

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

@johncowen johncowen added theme/ui Anything related to the UI pr/no-changelog PR does not need a corresponding .changelog entry labels Sep 8, 2022
Copy link
Contributor

@LevelbossMike LevelbossMike left a comment

Choose a reason for hiding this comment

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

I don't see this class being used anywhere in the codebase. Why is this change necessary? I understand the need for consistency when we want to implement something similar, but when we don't need this class in the codebase currently, why would we want to add this?

@johncowen
Copy link
Contributor Author

johncowen commented Sep 12, 2022

Why is this change necessary? I understand the need for consistency when we want to implement something similar

Well I suppose mostly the need for consistency 😄 , to be honest with you I thought we'd done this, but looked when we were talking about this helper offline and realized we hadn't.

Apart from the need for consistency, I can imagine future-me trying to do something, say for example hiding all peering related badges when peering is disabled with a CSS 1 liner using html:not(.has-peerings) and wondering why it doesn't work. I suppose now I've realised that we've not added this, future-me would remember that (maybe?) and re-add this line from this PR when it was needed, so I'm happy to close this if you feel we shouldn't add it right now. Just let me know.

Actually, partly related to the comment I just made on this PR (#14519 (comment)) - it might be an idea to add this capability/requirement to the more generic feature-flagging functionality so that we don't forget to add this class in the future - that way it would just happen without us even realizing.

Anyway, let me know if you want me to close this for now or not.

@johncowen
Copy link
Contributor Author

Closing this for the moment, we can circle back at a later date after #14518 is in and the Big Bang lint is done

@johncowen johncowen closed this Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-changelog PR does not need a corresponding .changelog entry theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants