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

SpreadsheetUI : Use ContextTracker #5992

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

johnhaddon
Copy link
Member

This lets us highlight the currently active row like so :

spreadsheetA

There is ample room for bikeshedding on the presentation, and opinions are welcome, but I am pretty sold on "context yellow" as a way of tying together all the upcoming context-aware features and linking with the existing colour for the current-frame indicator. @tomc-cinesite had some concerns that it could be confused for a warning, but in context I think it works pretty well, and used sparingly yellow works well to draw the eye without overwhelming. I don't know what other colour we'd use.

Tom also suggested a few alternative presentations, which I've tried out below...

Adding an "overline" to the underline

To avoid any ambiguity about what the underline is associated with. I don't mind this one, but I'm not sure the underlining is too ambiguous, especially once you've seen it update in context once or twice.

spreadsheetB

Using background colour

I strongly dislike this on two counts :

  • We're overloading background colour already, using it differentiate both even/odd rows and also disabled cells.
  • When faded to a suitable luminance for the background, the colour just gets muddy.

spreadsheetC

Using left/right lines

I find this much harder to read than any other presentation. Note that we do need to highlight the left/right of every cell rather than just the row ends, because disabled cells don't get highlighting.

spreadsheetD

@johnhaddon johnhaddon self-assigned this Aug 6, 2024
@tomc-cinesite
Copy link
Contributor

Thanks for exploring those presentations @johnhaddon. With you on your conclusion (though I might perhaps not mind the background one, if we werent using that in so many other ways already...)

The only other mystery option 'e' I could think of was something like:

ss2

@johnhaddon
Copy link
Member Author

The only other mystery option 'e' I could think of was something like

I tried something similar to that, highlighting the existing "vertical header" boxes on the left hand side. Aesthetically I think it's fine, but I think it also has a couple of downsides. It doesn't read as easily as the other options for very wide spreadsheets, because the indicator may be far from the cells you are looking at. And it might also be construed to mean that disabled cells are active, whereas the absence of underlining on those hopefully makes it clear that they're not :

image

@tomc-cinesite
Copy link
Contributor

I tried something similar to that, highlighting the existing "vertical header" boxes on the left hand side. Aesthetically I think it's fine, but I think it also has a couple of downsides. It doesn't read as easily as the other options for very wide spreadsheets, because the indicator may be far from the cells you are looking at. And it might also be construed to mean that disabled cells are active, whereas the absence of underlining on those hopefully makes it clear that they're not :

Yeah, I guess it depends on how much you want to re-enforce the activeness. The implemented version looks good 👍

Copy link
Contributor

@murraystevenson murraystevenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks John! Changes look good to me.

Given the options on this jaunt around the bike shed, I'm also happiest with the original underline. It seems the option with the least downsides and once we start incorporating "context yellow" elsewhere in the UI its purpose should become a bit more obvious...

This lets us highlight the currently active row.
@johnhaddon johnhaddon merged commit e5d1448 into GafferHQ:main Aug 7, 2024
6 checks passed
@johnhaddon johnhaddon deleted the focusAwareSpreadsheet branch August 7, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Pending release
Development

Successfully merging this pull request may close these issues.

3 participants