fix: highlighted cell synced background #181
Conversation
| @@ -38,10 +45,11 @@ | |||
| background: var(--colors-grey300); | |||
| } | |||
|
|
|||
| .invalid:not(.highlighted) { | |||
| .invalid:not(.active) { | |||
There was a problem hiding this comment.
Using the highlighted-selector prevented the highlighted-but-unfocused cell to not show the red-background.
|
🚀 Deployed on https://pr-181--dhis2-data-entry.netlify.app |
Birkbjo
requested review from
HendrikThePendric,
KaiVandivier and
tomzemp
and removed request for
HendrikThePendric
There was a problem hiding this comment.
I think the changes you have made here and the rationale behind them are correct. So we can go ahead and merge this in. This visual bug was a regression from #142 where I introduced the highlighted cell state for cells without focus.
Whilst testing the branch via the PR link, I noticed a few odd things in this cell UI. I'm pretty sure they weren't introduced in this PR, but might be something for @cooper-joe to consider....
Below is a cell into which I first entered a valid value, which was then synced. I then changed the value to a string which is invalid. Now the cell is "synced and errored". Technically correct, but users will get confused....
Perhaps here we should hide the sync indicator? (Not sure...)
And below is that same "synced and errored" cell again, but now I have focussed the input. In this case we have a cell with an invalid value, and we have a tooltip above which indicates why the value is invalid. However, since there is no red color, and we're even seeing some green from the sync indicator, I don't think the user can see very easily that this cell is invalid right now...
Perhaps it would make sense to show a red outline in this case?
|
I've a PR up to fix some of the conflicting cell-states you are seeing here #182 . This simply resets the "synced"-status whenever the value changes. I think that makes sense, since now the value does not match the last synced value.
I think it is intentional that the current focused field should not show a red background, atleast that was my understanding from the css, and I implemented the same behaviour here. However, we could maybe change the outline to red in that case?
I think this is about the same - we do show the tooltip above, but we don't show a glaring red-field until the user focuses another field, and I think we should keep that behaviour so we don't shout at the user before they have a chance to do the correct thing. |
No because then we don't see the highlighted cell anymore when interacting with the sidebar. |
Birkbjo
force-pushed
the
fix/highlighted-cell-synced-background
branch
from
d853339 to
a3dc51c
Compare
Yeah, I think hiding the sync indicator makes sense here. The invalid value is not synced, so a sync indicator is false signal.
The cell did have a red border/outline before the user focused it. I think showing the user that they have focused the cell is important, as they will hopefully already be aware of the error state from the tooltip and previous red background/outline. In isolation, it looks confusing, but I'm thinking it'll be clear in the context of using the app for real data entry. |
Birkbjo
force-pushed
the
fix/highlighted-cell-synced-background
branch
from
a3dc51c to
a20f882
Compare
* fix(cell): fix synced background for highlighted cell * fix: fix highlighted styling * fix: minor cleanup
Fixes some styling issues with highlighted field. Previously, the
!importantkeyword on highlighted-field preventedinvalidandsyncedstyles from being applied to the highlighted field when this was unfocused.I think we want the
white-background when editing the field - and this is now done by using the.active-selector, which is in my opinion more correct than using thehighlighted-field-selector.Highlighted-synced unfocused:
Before:
Highlighted-synced focused:
(same as before)
Highlighted-error unfocused:
Before:
