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

ColorPicker remove unused children and fix sample bugs. #101100

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

WhalesState
Copy link
Contributor

@WhalesState WhalesState commented Jan 3, 2025

Remove an unused VBoxContainer.
Remove a redundant VBoxContainer and add it's children directly to main vbox.
Remove 3 unused children from grid (Label, Slider, SpinBox).
Fix current color overbright position.
Added color mode buttons and sliders to a another VBoxContainer to hide both when sliders_visible is false.
Use tab_hovered instead of tab_pressed when hovering a mode button.

@WhalesState WhalesState requested review from a team as code owners January 3, 2025 22:06
@KoBeWi
Copy link
Member

KoBeWi commented Jan 3, 2025

Pretty sure changing base type breaks compatibility, which is why the redundant VBoxContainer had to exist.

@WhalesState
Copy link
Contributor Author

Pretty sure changing base type breaks compatibility, which is why the redundant VBoxContainer had to exist.

What is the use case of the MarginContainer ? if for the color button popup, then why don't we include the margins inside the popup's panel ?

@WhalesState
Copy link
Contributor Author

Reverted the base class to VBoxContainer again, it's easier to be reviewed now.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 3, 2025

What is the use case of the MarginContainer?

The "use case" is that ColorPicker doesn't need to be used with popup. It's a stand-alone node, ColorPickerButton only wraps it.
The container was added in #78468

@AThousandShips AThousandShips added this to the 4.4 milestone Jan 4, 2025
@WhalesState WhalesState force-pushed the color-picker-1 branch 2 times, most recently from e056cd0 to e44ae88 Compare January 9, 2025 06:57
@WhalesState WhalesState requested a review from a team as a code owner January 17, 2025 14:30
@WhalesState
Copy link
Contributor Author

Check the title, should i open a new issue for each fix to track them?

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Jan 17, 2025

There is a behavior change, which is questionable. Everything else looks fine.

Check the title, should i open a new issue for each fix to track them?

I think it's ok, but the sliders_vbox deserves its own issue/PR.

scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
Remove an unused `VBoxContainer`.
Remove 3 unused children from grid (Label, Slider, SpinBox).
Ignore sample input when old color is not displayed.
Fix current color overbright position.
@WhalesState
Copy link
Contributor Author

Applied all the suggestions.

@Repiteo Repiteo merged commit ecc8d28 into godotengine:master Jan 20, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 20, 2025

Thanks!

@WhalesState WhalesState deleted the color-picker-1 branch January 20, 2025 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pressing the ColorPicker's sample left half side changes the color to black when old color is not displayed.
4 participants