New song select footer design implementation #21491
Conversation
frenzibyte
changed the title
| protected override bool OnMouseDown(MouseDownEvent e) => !Enabled.Value || base.OnMouseDown(e); | ||
| protected override bool OnClick(ClickEvent e) => !Enabled.Value || base.OnClick(e); |
There was a problem hiding this comment.
I'm very confused by these conditionals. I do not think they should exist.
There was a problem hiding this comment.
what about them to be exact? i took this design pattern from the old footer buttons that were recently updated. although i did inline it prompted by the jetbrains tips, if thats the issue i can revert it, just making sure
There was a problem hiding this comment.
I'm not sure what the purpose is. Can you explain the purpose? If you can't, then it's probably not a good sign.
One rule to follow is to not blindly copy code, even if you are working with existing code. I believe these conditionals are going to make mouse clicks travel through the button to elements behind them when the buttons are not enabled, which is probably incorrect. I'd expect them to always be blocking mouse down / clicks?
| new Box | ||
| { | ||
| RelativeSizeAxes = Axes.Both, | ||
| Colour = colour.B5 |
There was a problem hiding this comment.
Not sure about this one. But it matches the design so let's go with it for now. I've left a comment in figma questioning its validity.
There was a problem hiding this comment.
This should be colourProvider.Background5.
peppy
force-pushed
the
footer_V2_implementation
branch
from
06c27e7
to
48f7e01
Compare
| new Box | ||
| { | ||
| RelativeSizeAxes = Axes.Both, | ||
| Colour = colour.B5 |
There was a problem hiding this comment.
This should be colourProvider.Background5.
…ur`. Remove unnecessary `FillFlowContainer`
Adjust shadow radius value to 5 to match figma.
pass ColourProvider in from test, instead of hard coding it in `FooterButtonV2.cs`
Co-authored-by: Joseph Madamba <madamba.joehu@outlook.com>
|
thanks for all the nitpicks btw joehuu, i need to be more careful |
| public Action NextRandom { get; set; } = null!; | ||
| public Action PreviousRandom { get; set; } = null!; |
There was a problem hiding this comment.
Generally we always make Actions Action? and support nullability, as a pattern.
supersedes #20148 and is part of #18890.
Cleaner commit history and isolated test only implementation to allow for merging without concern for other components it needs to work with.
line count is majorly bloated due to it being made as a drag and drop replacement, which is also the reason I put it all in one folder