-
Notifications
You must be signed in to change notification settings - Fork 11
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
269 group defences #318
269 group defences #318
Conversation
@heatherlogan-scottlogic Just noticed a bug, working on it now |
b40ac36
to
7ec146f
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.
Mostly concerns around your new collapsible control not being a React component, plus a few other minor things.
/* | ||
* Modified from https://www.w3schools.com/howto/howto_css_switch.asp | ||
*/ | ||
|
||
/* The switch - the box around the slider */ | ||
.switch { |
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.
Can't help thinking a checkbox would be so much simpler than a homegrown slider. This is a whole heap of css...
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.
This bit isn't really added in this PR, I was just moving the comment to a more suitable place. Could maybe put it in it's own component to hide it away? But I'd prefer for that work to be in a new ticket as it's not really to do with this 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.
Yes understood! Was just a comment in passing. Think we could add an issue for it though, it's really unwise to overcomplicate things. If we are going to overhaul the UI, we might consider a component library instead of home-cooking all our own.
For the collapsible sections in the control panel
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.
Excellent!
))} | ||
</div> | ||
)} | ||
</article> |
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.
Oh this is looking much better, nice use of article
👍
* Grouped defences * Better html grouping * Removed strategy box * Visually clearer grouping * Rounded corners on control section * Using some rather than find * Using details html elements For the collapsible sections in the control panel * Better html DRY principles
No description provided.