[93] Fixes colors breaking after value is cleared and saved

squarewave17 commented 3 months ago

Description

Fixes #93 - addresses feedback given Here

Note

Although the bug has been fixed and the user can no longer get into a situation where they can't select a colour, the colour picker still can't be cleared if a default value exists. I believe fixing this will require a rethink of how we are handling data. A proposal for handling data more efficiently can be found HERE(soon)

Change Log

Adds a new utility function - removeEmptyValues
Removes any empty values before setUserConfig is dispatched

Steps to test

You can follow http://bigbite.im/v/WSCUlJ to test however, the problem will not be apparent if any default data exists. I was only able to recreate this issue on blocks. See comments in the description for further info

Go to a block, set one of the colours and save it
Now clear the colour picker and see the value should be completely removed from the code view
Save the empty state
Now select a colour again and everything should work as expected

Screenshots/Videos

https://github.com/user-attachments/assets/0fd1c5d1-eb93-4315-843b-d317c7903a54

Checklist:

[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[x] I have commented my code, particularly in hard-to-understand areas
[x] I have made corresponding changes to the documentation
[x] My changes generate no new warnings
[x] Any dependent changes have been merged and published in downstream modules

g-elwell commented 3 months ago

Thanks for this @squarewave17 !

Although the bug has been fixed and the user can no longer get into a situation where they can't select a colour, the colour picker still can't be cleared if a default value exists.

I've tested this PR and it works with this caveat. However, there is an option for us to overwrite the value with an empty value, rather than unsetting it. This enables 'clearing' the value and also allows users to override existing values, if present.

https://github.com/user-attachments/assets/9e698e83-82c1-47fc-a0de-fcc5a0841488

The only downside to this approach is you may end up in a situation where you have several unnecessary empty values within a theme.json file, but perhaps this could be addressed later, as the change required to get this working initially would be very minor - we just need to ensure the colour falls back to an empty string instead of undefined when cleared:

// StylesColor.js L31
hexToVar( newValue, themePalette ) ?? ''

Interested to know your thoughts on this approach as it could avoid some complexity (esp. if the alternative involves rethinking how we handle data).

squarewave17 commented 3 months ago

Thanks for this @squarewave17 !

Although the bug has been fixed and the user can no longer get into a situation where they can't select a colour, the colour picker still can't be cleared if a default value exists.

I've tested this PR and it works with this caveat. However, there is an option for us to overwrite the value with an empty value, rather than unsetting it. This enables 'clearing' the value and also allows users to override existing values, if present.

Screen.Capture.on.2024-08-23.at.11-01-53.mp4 The only downside to this approach is you may end up in a situation where you have several unnecessary empty values within a theme.json file, but perhaps this could be addressed later, as the change required to get this working initially would be very minor - we just need to ensure the colour falls back to an empty string instead of undefined when cleared:
// StylesColor.js L31
hexToVar( newValue, themePalette ) ?? ''
Interested to know your thoughts on this approach as it could avoid some complexity (esp. if the alternative involves rethinking how we handle data).

That works for me, great solution. Can we definitely still add a colour once it's been cleared, as that's not clear from the demo?I imagine it would be fine though as the empty string is keeping it stable.

g-elwell commented 3 months ago

Thanks for this @squarewave17 !

Although the bug has been fixed and the user can no longer get into a situation where they can't select a colour, the colour picker still can't be cleared if a default value exists.

I've tested this PR and it works with this caveat. However, there is an option for us to overwrite the value with an empty value, rather than unsetting it. This enables 'clearing' the value and also allows users to override existing values, if present. Screen.Capture.on.2024-08-23.at.11-01-53.mp4 The only downside to this approach is you may end up in a situation where you have several unnecessary empty values within a theme.json file, but perhaps this could be addressed later, as the change required to get this working initially would be very minor - we just need to ensure the colour falls back to an empty string instead of undefined when cleared:
// StylesColor.js L31
hexToVar( newValue, themePalette ) ?? ''
Interested to know your thoughts on this approach as it could avoid some complexity (esp. if the alternative involves rethinking how we handle data).
That works for me, great solution. Can we definitely still add a colour once it's been cleared, as that's not clear from the demo?I imagine it would be fine though as the empty string is keeping it stable.

Yeah, it lets you re-add a colour as well. I've opened a PR here so it can be tested https://github.com/bigbite/themer/pull/100

g-elwell commented 3 months ago

Closing as https://github.com/bigbite/themer/pull/100 has been merged which resolves this for now

bigbite / themer