Page MenuHomePhabricator
Differential D4471

[web] Fix app switcher for thread creation
AbandonedPublic
Actions

Authored by • jacek on Jul 7 2022, 5:34 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 27, 6:43 AM
Unknown Object (File)
Fri, Dec 27, 6:43 AM
Unknown Object (File)
Fri, Dec 27, 6:42 AM
Unknown Object (File)
Fri, Dec 27, 6:37 AM
Unknown Object (File)
Sat, Dec 7, 9:00 PM
Unknown Object (File)
Nov 12 2024, 1:13 AM
Unknown Object (File)
Nov 12 2024, 1:13 AM
Unknown Object (File)
Nov 12 2024, 1:13 AM
View All Files
Subscribers
• abosh
• adrian
ashoat

Details

Reviewers
tomek
atul
• abosh
Summary

Currently app switcher highlights current app (on the left side) using navTabSelecter. The change allows highlighting chat icon in both chat and chat-creation navigation state.

After (in chat creation state):

Screenshot_Google Chrome_2022-07-07_150114.png (200×249 px, 6 KB)

Test Plan

Enter chat creation mode. The chat icon on the left should remain highlighted.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• jacek created this revision.Jul 7 2022, 5:34 AM
Herald added subscribers: • abosh, • adrian, ashoat. · View Herald TranscriptJul 7 2022, 5:34 AM
Harbormaster completed remote builds in B10347: Diff 14255.Jul 7 2022, 5:49 AM
• jacek requested review of this revision.Jul 7 2022, 5:49 AM
• jacek added a parent revision: D4457: [web] Introduce `chat-creation` mode in `navInfo`.Jul 7 2022, 5:57 AM
• jacek added a child revision: D4465: [web] Support chat creation in `ChatMessageListContainer`.
• jacek edited the summary of this revision. (Show Details)Jul 7 2022, 6:01 AM
• abosh requested changes to this revision.Jul 7 2022, 7:00 AM
• abosh added inline comments.
web/selectors/nav-selectors.js
17 ↗(On Diff #14255)

Why was this change made? Flow/ESLint wasn't complaining and it doesn't match the syntax in the rest of the imports.

218 ↗(On Diff #14255)

This is a bit pedantic, but looks cleaner. Not necessary to implement, though.

This revision now requires changes to proceed.Jul 7 2022, 7:00 AM
• abosh added inline comments.Jul 7 2022, 7:45 AM
web/selectors/nav-selectors.js
218–222 ↗(On Diff #14255)

Actually, in a follow up to my above comment, this can also be shortened with a ternary.

• jacek added inline comments.Jul 8 2022, 4:16 AM
web/selectors/nav-selectors.js
17 ↗(On Diff #14255)

Interesting, I really don't know. Probably somehow by accident. I'll revert it.

218 ↗(On Diff #14255)

Not sure which is better,
It could be a bit dangerous, because if we introduce some new tab starting with chat in the future, this check would automatically cover it, what may not be the best solution.

• abosh accepted this revision.Jul 8 2022, 7:17 AM
• abosh added inline comments.
web/selectors/nav-selectors.js
218 ↗(On Diff #14255)

That's a great point. In that case, I don't think it's necessary to incorporate this suggestion.

This revision is now accepted and ready to land.Jul 8 2022, 7:17 AM
• abosh requested changes to this revision.Jul 8 2022, 7:18 AM

accidentally hit accept, sorry. Will accept once this change is reverted.

This revision now requires changes to proceed.Jul 8 2022, 7:18 AM
• jacek planned changes to this revision.Jul 11 2022, 1:58 AM
• jacek updated this revision to Diff 14385.Jul 11 2022, 2:33 AM

revert accidental import change

Harbormaster completed remote builds in B10441: Diff 14385.Jul 11 2022, 2:45 AM
• jacek abandoned this revision.Jul 11 2022, 6:44 AM

Abandoning due to changes in previous diffs that eliminated the need for the change here.

Revision Contents
Changeset List

PathSize
web/
selectors/
4 lines

Diff 14385

View Options

web/selectors/nav-selectors.js

Loading...