Skip to content

web: change Sidebar tab semantics#4116

Merged
nicks merged 1 commit into
masterfrom
nicks/ch10949
Jan 25, 2021
Merged

web: change Sidebar tab semantics#4116
nicks merged 1 commit into
masterfrom
nicks/ch10949

Conversation

@nicks

@nicks nicks commented Jan 25, 2021

Copy link
Copy Markdown
Contributor

Hello @hyu, @landism,

Please review the following commits I made in branch nicks/ch10949:

0415af8 (2021-01-25 11:50:45 -0500)
web: change Sidebar tab semantics
now, we never change the number of tabs open if the selected resource is already open

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@nicks
nicks requested review from hyu and landism January 25, 2021 16:51
@shortcut-integration

Copy link
Copy Markdown

Comment thread web/src/TabNav.tsx Outdated
// it opens a new tab, and brings the new tab into focus.
// 2) Otherwise, when you select a resource on the left sidebar,
// a) click opens it in the current tab,
// b) ctrl-click opens it in a new tab on the immediate right of current tab

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not part of this PR, but I noticed during review:

For me on Firefox, the Mac OS context menu still has precedence. I assume the expected behavior is that Tilt should hijack?

(Maybe I'm missing something. You're able to override the context menu on Mac?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yep, on macos it's the command key

fwiw, most of the standards docs on this tend to be super vague and hand-wavey for exactly the reason you describe - this tends to be dictated by a complicated set of overlapping OS-level preferences and browser-level preferences, so i changed this comment to be more hand-wavey

now, we never change the number of tabs open if the selected resource is already open

@landism landism left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so many fewer cases!

@nicks
nicks merged commit f6f874e into master Jan 25, 2021
@nicks
nicks deleted the nicks/ch10949 branch January 25, 2021 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants