silverweed
Added Tier Color Changing
When selecting a tier, you can change the label and with this PR, change the color. Color values are saved to the json file and will be imported when loading the json file.
Additionally, the tiers are opened by selecting their container, and closed by selecting the container a second time.
Hello, thanks for the contribution. I tested this quickly and found 2 problems: 1- if you click on a row label, don't change anything and click outside, the edit input doesn't close as it should; 2- if you click on the title, don't change anything and click outside, the text becomes misaligned.
(tested on Firefox)
Could you please make sure these problems are solved before I take a more detailed look at the changes?
1- if you click on a row label, don't change anything and click outside, the edit input doesn't close as it should;
This was more of a design choice because the "focusout" event listener was being triggered when the color picker dialog opened. I have re-evaluated the code and made changes to ensure focusout could be re-added, so now the header can close without needing to click it.
2- if you click on the title, don't change anything and click outside, the text becomes misaligned.
This was a bug, and should now be fixed.
Also, if now I click on a row the color defaults to red no matter which row it is, and if I click outside without touching anything the row changes to red. Of course it should instead keep the current row color.
There was another bug: when adding a new row, the header colors are reset. This is due to the recompute that runs. Right now, my solution was to make the recompute only run for the new row on certain instances by passing the row index. I'm not sure if this is gives us the desired effect. Please let me know if you have a better suggestion. Thank you.
Thanks for the changes. It's almost ready, but I found another bug: if you open a tier, enter a new name, then before closing it you open the color picker, you lose the changes to the name. Another small issue: if you have the color picker open and you click outside to close the tier edit box, the color picker will stay open. Do you think you can fix these issues before I merge?
Thanks for the changes. It's almost ready, but I found another bug: if you open a tier, enter a new name, then before closing it you open the color picker, you lose the changes to the name. Another small issue: if you have the color picker open and you click outside to close the tier edit box, the color picker will stay open. Do you think you can fix these issues before I merge?
I fixed the first issue you presented in the latest commit.
Additionally, the focusout logic didn't work with chromium-based browsers, as the element attribute I was relying on was only present in Firefox. I have made changes to unify the code so it works with chromium and gecko-based browsers. Alternatively, I can keep the code I had previously and use a conditional to use my chrome-based logic or gecko-based logic. To get the logic that works with chrome working with Firefox, I used a grace period timer, but overall the functionality is now the same or better and doesn't rely on where the user clicks (such as the empty row space or images).
The color picker is also different with chromium browsers. There isn't an "Ok" button so it's unaffected by the second issue you presented.
For the second issue, could you provide more detail? When I open the color picker on Firefox (Windows OS) it takes the focus permanently within the tab. I can't close the header or click away, it's an open dialog box that must be closed manually.
@gccalvin thanks for the fix: the first issue seems solved now also on my browser. As for the second one: I'm using Librewolf (basically Firefox) on Linux with a tiling window manager, and it allows me to unfocus the browser even when the color picker is open. If I do, it will close the tier editor but not the picker itself. As mine is a quite uncommon setup and the problem is not really a big deal, I think we can merge in this state rather than try and implement a potentially complicated and platform-dependent fix. I'd rather keep the code as simple as possible. What do you think?
@silverweed Agreed, the issue is more corner-case. In the unlikely event another user reports this with Librewolf or other browsers, it can be revisited. You can proceed with merging this. Thanks.