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

Add right-handed arrow to view title to emphasize the focused view #4024

Merged
merged 15 commits into from
Nov 2, 2023

Conversation

carolinebridge
Copy link
Contributor

summary of changes

  • bolds view title and view title bar icons when the view is in focus
  • assigns a focus view id if a change has been made to the focus views length (i.e. a view has been closed or opened)

preview:

image

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Oct 27, 2023
@carolinebridge carolinebridge added enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Oct 30, 2023
@carolinebridge
Copy link
Contributor Author

Partial resolutions to #3862 and #3860 ;; response to #3870

@@ -62,8 +62,8 @@ test('using the hotkey to bookmark the current region', async () => {

// @ts-expect-error
const { bookmarks } = session.widgets.get('GridBookmark')
expect(bookmarks[0].start).toBe(100)
expect(bookmarks[0].end).toBe(140)
expect(bookmarks[0].start).toBe(105)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this expected to be changed? could be related to #3996 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should likely do a full snapshot of the bookmarks object for test

@cmdcolin
Copy link
Collaborator

stylistically, I am not sure am +1 on the bold font. it just looks a little odd to me

@cmdcolin
Copy link
Collaborator

cmdcolin commented Oct 30, 2023

random thoughts on the matter:

I know it is hard to try to truly "call attention" to the thing that is "focused" but also, I have questioned: what is the motivation behind trying to call so much attention in the first place? If we compare to other UI systems, the focus can often be pretty subtle. Other systems are not always a very bold about what is in focus.

The comparisons are sort of hard to draw though, and our UI challenges are somewhat unique because we have a "stack of views" instead of a floating windowing system like an OS has, or tabs in a browser. I think the vertical stack of views is actually kind of a challenge to convey "focus" properly, and a true windowing system conveys focus much more naturally (e.g. just check whats at the top of the stack). I have even run into a need for floating windows with e.g. the protein view: the screen easily becomes "too tall" to use properly, while my desired behavior would be having a protein view hovering to the right of the screen instead of stacked vertically.

If and when we get that type of windowing system, I think there will be more natural analogs to other 'focus' systems common in different other apps and OS's

@carolinebridge
Copy link
Contributor Author

^ Agree with the challenges for stacks of views. The motivation behind enhancing the focus is that it is hard for some to perceive that there is any focus at all with the difference between the .main and .dark palette options, alone.

I think relying on colour alone to distinguish between object features may be naiive...I think in previous UI discussions we've had a lot of success combining colour indicators with coordinating indicators in shape and intensity.

Some of the more subtle "out-of-focus" stylings convey "disabled", and a bright border is too harsh, which is why I went for bolding the existing UI. I found it caught the eye without dismissing the other views.

Do you think the caret alone is enough to indicate the focus?

@cmdcolin
Copy link
Collaborator

Do you think the caret alone is enough to indicate the focus?

Yes, I think it is probably sufficient to just have caret, and I definitely I approve the caret:)

here is a before and after

with bold

image

without bold

image

@carolinebridge
Copy link
Contributor Author

I'll make those changes

Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #4024 (6c5d8a4) into main (4fe46f8) will increase coverage by 0.01%.
The diff coverage is 92.00%.

❗ Current head 6c5d8a4 differs from pull request most recent head 86730a7. Consider uploading reports for the commit 86730a7 to get more accurate results

@@            Coverage Diff             @@
##             main    #4024      +/-   ##
==========================================
+ Coverage   63.68%   63.69%   +0.01%     
==========================================
  Files        1026     1026              
  Lines       30251    30271      +20     
  Branches     7199     7210      +11     
==========================================
+ Hits        19264    19281      +17     
- Misses      10822    10825       +3     
  Partials      165      165              
Files Coverage Δ
packages/app-core/src/AppFocus/index.ts 100.00% <100.00%> (ø)
packages/app-core/src/ui/App/ViewContainer.tsx 92.30% <85.71%> (-2.94%) ⬇️
packages/app-core/src/ui/App/ViewHeader.tsx 86.66% <85.71%> (+4.84%) ⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin cmdcolin force-pushed the emphasize-focus-view branch from 2c784dc to 86730a7 Compare November 2, 2023 22:10
@cmdcolin cmdcolin merged commit 7ff2095 into main Nov 2, 2023
11 checks passed
@cmdcolin cmdcolin deleted the emphasize-focus-view branch November 2, 2023 22:15
@cmdcolin
Copy link
Collaborator

cmdcolin commented Nov 2, 2023

@carolinebridge-oicr I went ahead to with merge but I modified it to remove autofocusing view after one is closed, and deferred that into a follow up issue here #4036

There are a lot of tricky issues to solve with the focusing UI, especially with the subviews, and I wanted to try to avoid hardcoding the search for view.views especially as that assumption, if it was not valid, could result in the focusing not working at all (since the search would not find anything, and then apply the next autofocus)

@cmdcolin cmdcolin changed the title Updates view in focus styling to be more emphasized and ensures a view is always in focus if one is open Add right-handed arrow to view title to emphasize the focused view Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants