-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
MudColorPicker: Make color field resizable. Closes: #8717 #8744
base: dev
Are you sure you want to change the base?
MudColorPicker: Make color field resizable. Closes: #8717 #8744
Conversation
The height and width of the color field is hardcoded to 250px and 312px respectively in _pickercolor.scss. This style will override that is height/width is specified. Also centered color field.
…ker.razor.cs Two new public variables: ColorFieldHeight, ColorFieldWidth for setting height/width (_maxX and _maxY) of the color field.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #8744 +/- ##
==========================================
+ Coverage 89.82% 90.10% +0.27%
==========================================
Files 412 419 +7
Lines 11878 12192 +314
Branches 2364 2396 +32
==========================================
+ Hits 10670 10986 +316
+ Misses 681 668 -13
- Partials 527 538 +11 ☔ View full report in Codecov by Sentry. |
Could you add tests to make sure it works the same across different sizes? Controls should stay centered. I think we should consider the ability to use relative scaling (ex: Would it be possible as a user to style it entirely in CSS so you could have it stretch to the viewport without having to specify the dimensions? Does it impact any other modes than Spectrum? |
Yes I can add tests.
I will look into the css to do so.
That sounds like a good addition, but I would probably need help/examples/more research.
The problem with only doing this with CSS is that visually it would work, but the range of colors is driven by the _maxX/_maxY variables. (See first image in #8717)
Not that I can find. _maxX/_maxY is only used when doing things with the selector in the color field spectrum or when the color is set (but only to position the selector) |
I found it actually does affect other modes (grids and palette) looking into fixes that would either exclude those from being affected or find ways to make them resize properly. |
Fixes cases when width of color picker view is over 312px
do you think it would be difficult to re-architecture it in a way that it is not dependent on pixels to calculate the colors? then we could do things like have a size property or scale it by font size also I think it could be worth having the entire controls along with the field scale together in that case |
get => _maxY; | ||
set | ||
{ | ||
_maxY = value; |
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.
Such code is forbidden since v7 started and we slowly getting rid of logic in parameter setters and getters, because of this:
dotnet/aspnetcore#26230 (comment)
Read https://github.com/MudBlazor/MudBlazor/blob/dev/CONTRIBUTING.md on how to use ParameterState framework + use other components that already using it for the reference and real usage.
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.
Description
Added height/width public properties (that update private _maxX/_maxY) so the color field spectrum can be resized. Before this PR, changing the css for the color field would only visually update the color field, and it would not let you use the full range of colors (details in issue). Also changed color field css to have margin-left/right 50% so the color field is centered.
When going past the default 312px width the ui elements below the color field are aligned left, do we want a different behaviour? (see in demo gif below)
Closes #8717
How Has This Been Tested?
Visually tested in docs. Added example on docs page.
Type of Changes
Checklist
dev
).