Differential D3980
[web] Fix `ThreadSettingsGeneralTab` save button `disabled` prop Authored by • abosh on May 9 2022, 9:50 PM. Tags None Referenced Files
Details Fixed the save button in ThreadSettingsGeneralTab which was previously not staying disabled if the user had selected another color and then changed their selection back to the current thread color (making no other changes in the thread settings) (see before/after video in Test Plan). Related Linear issue with full context here. Tested on Chrome/Safari and works as expected. Before:
Diff Detail
Event Timeline
Comment Actions Removed case when queuedChanges.name and queuedChanges.description are '' (if the Comment Actions This logic is pretty verbose and will only grow in size with the addition of new settings. I think it'd be cleaner to handle the issue with the color setting in onChangeColor. What do you think? Comment Actions Ok, so the previous issue was that something like #ffffff was being compared with something like ffffff and so changes were always queued? And using tinycolor, which handles either way of representing hex string, gets around that issue? Should we instead be consistent about using hex colors that start with # vs without? Or maybe we should use always run the strings through tinycolor to figure things out for us? Either way, looks like this resolves the issue. (Adding @ashoat/@palys-swm as reviewers to see if they have thoughts) Comment Actions I think relying on us being consistent with hex values is a great idea, but we should still run the strings through tinycolor for the guarantee that color equality is handled the same way each time. For example, someone could easily forget to add or remove a # from the string but not know it, introducing a bug like this one. Using tinycolor.equals() removes this aspect of human error. Comment Actions
Yes, I think so. Can you clarify where the mismatch is? In some places we use #${color} to convert between... maybe we need to do that here too? Not opposed to using tinycolor but would like more context on what's causing the issue to determine if a simple string concatenation somewhere would be a more appropriate solution. Comment Actions Would this be a programmer forgetting to remove a #, or a user? I assume it's the former since I don't think we let the user input hex codes anywhere... as for the programmer, I wonder if there would be a way to enforce this with Flow. Comment Actions Here's the exact issue, it's actually not related to the #: The color of the thread (threadInfo.color) is stored as a lowercase hex string (screenshot is from Redux DevTools, which I used to confirm this):  However, when the user selects another color (updating queuedChanges), queuedChanges.color is stored as an uppercase hex string (screenshot is from React Developer Tools):  Now, let's say the user switches their selection back to the original thread color. So now queuedChanges.color is the uppercase version of threadInfo.color, when really it should be undefined since no "change" was made! But in the original code, queuedChanges.color is the uppercase version of threadInfo.color, which doesn't disable the Save button since there was some change queued. This is all because the original equality check will return false since it's checking for string equality, which means checking two strings against each other, case-sensitively. However, hex strings should be compared case-insensitively, and moreover, any strings representing colors should probably have their own special form of equality checking to ensure they're the same color (like converting to an RGB string or something). In this case, though, if we're all only using hex strings for colors this isn't necessary. The usage of tinycolor just ensures this color equality in a standardized way. Comment Actions Thanks for the detailed explanation, @yayabosh! Can we also make sure to lowercase the strings going into queuedChanges.color? Separate note: I'm not sure where else we do string comparisons of hex codes, but I suspect we have other places in the app where we do string comparisons without tinycolor. Comment Actions
Done!
I'll make a Linear issue to track this and try and get this fixed. There are other places in the codebase where we compare color strings without tinycolor. | |||||||||||||||||||||||||||||||||||||||||||