-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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: Create and use collapsible notices component #10270
Conversation
@@ -22,43 +22,93 @@ as |route|> | |||
</BlockSlot> | |||
</EmptyState> | |||
{{else}} | |||
|
|||
|
|||
{{#if (or (and topology.DefaultAllow topology.FilteredByACLs topology.undefinedIntention) (and topology.WildcardIntention topology.FilteredByACLs topology.undefinedIntention)) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈 The conditional works but I had to duplicate the code in the {{else}}
. LMK if you can think of a better way to format this. @johncowen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theres an option here to maybe make a computedProperty on the topology model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I've had a proper look at this after our chat, I guess I can think of a couple of options:
- I remember speaking to @freddygv about the all of these
boolean
type properties can quickly get out of hand, and I wondered if using some sort ofstate: ""
with a predefined list of states might have been a better approach here? From the condition you've written here it looks like there are two states, I'm not sure if thats correct or not as I don't have enough context into the feature to know. This one is obviously necessitates a backend change, and it might not even be possible/might be a bit late in the game to be tweaking this, but it is an internal endpoint so maybe not? - If the booleans are this 'nuanced', it looks to me like this is just an
or
checking all four fields, which is more straightforwards that what you have here, and that also lines up with how you've written the conditionals below.
{{or
topology.DefaultAllow
topology.FilteredByACLs
topology.undefinedIntention
topology.WildcardIntention
}}
Actually didn't we remove all the references to undefined
?
Me personally I like number 2 more than the computed property option, as this is a view concern not a model one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2nd option does not take into account that I have to have >2 notices to show that button. That's what I'm trying to capture in the conditionals I wrote. Grouping them all together in the or
does not guarantee that use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, of course yeah I forgot about that 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if you could do something with compact
or array_some
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this?
{{#let (without false (array true true false false)) as |visibleNotices|}}
{{if gt visibleNotices.length 2}}
{{!-- make collapsable --}}
{{/if}}
{{/let}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've already moved the logic to a computed func in the model. Does that work?
I also just pushed renaming undefinedIntention
to be notDefinedIntention
.
c03276d
to
f3c75d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I had this sitting here this morning and I hadn't submitted the review, just realized you pushed the computed version! Sorry!
Anyway, read through and see what you think.
@@ -22,43 +22,93 @@ as |route|> | |||
</BlockSlot> | |||
</EmptyState> | |||
{{else}} | |||
|
|||
|
|||
{{#if (or (and topology.DefaultAllow topology.FilteredByACLs topology.undefinedIntention) (and topology.WildcardIntention topology.FilteredByACLs topology.undefinedIntention)) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I've had a proper look at this after our chat, I guess I can think of a couple of options:
- I remember speaking to @freddygv about the all of these
boolean
type properties can quickly get out of hand, and I wondered if using some sort ofstate: ""
with a predefined list of states might have been a better approach here? From the condition you've written here it looks like there are two states, I'm not sure if thats correct or not as I don't have enough context into the feature to know. This one is obviously necessitates a backend change, and it might not even be possible/might be a bit late in the game to be tweaking this, but it is an internal endpoint so maybe not? - If the booleans are this 'nuanced', it looks to me like this is just an
or
checking all four fields, which is more straightforwards that what you have here, and that also lines up with how you've written the conditionals below.
{{or
topology.DefaultAllow
topology.FilteredByACLs
topology.undefinedIntention
topology.WildcardIntention
}}
Actually didn't we remove all the references to undefined
?
Me personally I like number 2 more than the computed property option, as this is a view concern not a model one.
12c243d
to
319ad3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Especially, thanks for adding the docs for the new component!
There are a couple of things here we could potentially circle back and tweak, but fine to do that in a PR in the future at some point.
f9fc233
to
52af9e7
Compare
* Create and use collapsible notices * Refactor collapsible-notices * Split up the topology acceptance tests * Add acceptance tests for tproxy notices * Add component file * Adds additional TProxy notices tests * Adds conditional to only show collapsable if more than 2 notices are present * Adds changelog * Refactorting the conditonal for collapsing the notices * Renaming undefinedIntention to be notDefinedIntention * Refactor tests
* Create and use collapsible notices * Refactor collapsible-notices * Split up the topology acceptance tests * Add acceptance tests for tproxy notices * Add component file * Adds additional TProxy notices tests * Adds conditional to only show collapsable if more than 2 notices are present * Adds changelog * Refactorting the conditonal for collapsing the notices * Renaming undefinedIntention to be notDefinedIntention * Refactor tests
* Create and use collapsible notices * Refactor collapsible-notices * Split up the topology acceptance tests * Add acceptance tests for tproxy notices * Add component file * Adds additional TProxy notices tests * Adds conditional to only show collapsable if more than 2 notices are present * Adds changelog * Refactorting the conditonal for collapsing the notices * Renaming undefinedIntention to be notDefinedIntention * Refactor tests
CollapsibleNotices
componentCollapsibleNotices
in Toplogy route when there are more than 2Notices
.