[web] Introduce navigation for enabled apps tab
ClosedPublic
Actions

Authored by • jacek on Feb 23 2022, 3:00 AM.

Details

Reviewers

tomek
• benschac
atul
ashoat

Commits

rCOMM26f86c398ee9: [web] Introduce navigation for enabled apps tab

Summary

Extend existing navigation to support new panel for apps management.
App Directory component in introduced in the following diff - it's only temporary mock to confirm that navigation works
Also introduced conditional Calendar tab rendering (as it will be optional app)

The video shows the tab after introducing App Directory component:

Screenshot_Google Chrome_2022-02-23_120355.gif (472×800 px, 225 KB)

Test Plan

Run web and check if navigation to Apps tab works correctly

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• jacek created this revision.Feb 23 2022, 3:00 AM

Herald added subscribers: • karol, atul, • adrian, ashoat. · View Herald TranscriptFeb 23 2022, 3:00 AM

Harbormaster completed remote builds in B6963: Diff 9827.Feb 23 2022, 3:04 AM

• jacek requested review of this revision.Feb 23 2022, 3:04 AM

• jacek edited the summary of this revision. (Show Details)Feb 23 2022, 3:06 AM

This diff does two things:

Handles new nav path
Makes chat link rendered conditionally

Please split this diff into two

This revision now requires changes to proceed.Feb 23 2022, 9:56 AM

Excluded conditional Calendar rendering into separate diff

Harbormaster completed remote builds in B7005: Diff 9878.Feb 24 2022, 1:14 AM

• jacek added a child revision: D3284: [web] Render calendar navigation tab conditionally.Feb 24 2022, 1:15 AM

Looks ok, but please clarify your intention regarding navInfoFromURL function

web/apps/apps-directory.react.js
6 ↗	(On Diff #9878)	We shouldn't use inline style here with color not from our theme, but this is replaced in the next diff. If we wanted to improve it, we could first implement `AppsDirectory` and then connect it to navigation. Let's keep the current approach for now, but we should use the proposed approach in the future.
web/url-utils.js
99–107 ↗	(On Diff #9878)	Not sure if this was intentional, but now the behavior is slightly different. So previously when neither chat not calendar was set in urlInfo, we used calendar. Now, when chat, calendar and apps are nulls, we use chat - was this change intentional? Was the intention to render chat by default?

This revision now requires changes to proceed.Feb 24 2022, 3:22 AM

• jacek added inline comments.Feb 24 2022, 3:34 AM

web/apps/apps-directory.react.js
6 ↗	(On Diff #9878)	Yes, I was wondering which diff sequence is better. I'll do next things following your proposed approach in the future.
web/url-utils.js
99–107 ↗	(On Diff #9878)	Yes, the change was intentional. As we wanted to make chat default tab, I assumed that there are no reasons to leave calendar default when extending this condition.

• jacek requested review of this revision.Feb 24 2022, 4:25 AM

• jacek added reviewers: • benschac, atul.

tomek accepted this revision.Feb 24 2022, 8:01 AM

tomek added inline comments.

web/url-utils.js
99–107 ↗	(On Diff #9878)	In that case it would be better to start with a diff that changes this behavior. At least, this change of logic should be marked by an inline comment to avoid the confusion.

This revision is now accepted and ready to land.Feb 24 2022, 8:01 AM

Herald added a reviewer: ashoat. · View Herald TranscriptFeb 24 2022, 8:01 AM

This revision now requires review to proceed.Feb 24 2022, 8:01 AM

tomek added inline comments.Feb 24 2022, 8:02 AM

web/apps/apps-directory.react.js
6 ↗	(On Diff #9878)	Ok. As a general rule it is usually better to first introduce something and then use it. Obviously that makes testing the diffs harder, but the benefit is that the master is always correct and working.

ashoat accepted this revision.Feb 24 2022, 10:43 AM

ashoat added inline comments.

web/url-utils.js
99 ↗	(On Diff #9878)	Nit: add a space above this for consistency with the rest
99–107 ↗	(On Diff #9878)	Yeah, agree that this should've been its own diff. We should avoid "sneaking in" big changes like this in unrelated diffs. That said, probably not worth factoring this out into its own diff at this point