Page MenuHomePhabricator

[web] Introduce Apps page
ClosedPublic

Authored by jacek on Feb 23 2022, 3:02 AM.
Tags
None
Referenced Files
F3295984: D3266.id10106.diff
Sun, Nov 17, 12:17 AM
Unknown Object (File)
Fri, Nov 15, 11:18 PM
Unknown Object (File)
Tue, Nov 5, 9:44 PM
Unknown Object (File)
Oct 14 2024, 2:43 AM
Unknown Object (File)
Oct 14 2024, 2:43 AM
Unknown Object (File)
Oct 14 2024, 2:43 AM
Unknown Object (File)
Oct 14 2024, 2:43 AM
Unknown Object (File)
Oct 14 2024, 2:42 AM

Details

Summary

Introduce Apps panel similar to existing in native app.
FontAwesome icons will soon be replaced by SWMIcons in follow-up diff.

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

Test Plan

Run web and confirm that switching calendar on and off works

Diff Detail

Repository
rCOMM Comm
Branch
jacek/apps-tab
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested changes to this revision.Feb 23 2022, 10:19 AM

On the video you provided it looks like plus button is rendered slightly lower than green ok button. (It might be just me, or the color, though) Please make sure that the button does not jump.

I know this code is based on what we have on native, but it doesn't look like reusing that code makes sense (most of the code is React Native). It would be great in the future if you could point out to the code that has the same purpose / is an inspiration of.

web/apps/app-listing.react.js
20–21

This is slightly confusing, as enabled flag should be determined by the parent (based on alwaysEnabled and enabledApps). I guess this approach was taken because alwaysEnabled is also used when determining if the button is clickable. It makes some sense, but what if we needed an app that is always disabled?
So I think that instead of alwaysEnabled we should rather have something like readOnly and enabled was always set to true by the parent when alwaysEnabled is true. What do you think?

22–24

I don't think we need to specify app prefix. We can just use name, icon and copy.

37

Please remove this console log

55

Shouldn't we use colors from our theme?

web/apps/apps-directory.react.js
33

I don't think this is correct. If we can't find an app in enabledApps then we shouldn't conclude that it is always enabled. Instead we should use alwaysEnabled flag.

This revision now requires changes to proceed.Feb 23 2022, 10:19 AM
web/apps/app-listing.react.js
20–21

The approach was taken because it's exactly the way it works in native app. I agree - the approach with enabled and readOnly is better.

22–24

I agree - I just used the names from native.

37

Sorry for missing it

55

Yes, I missed it

web/apps/apps-directory.react.js
33

Yes, I agree

Updates following Tomek's comments

benschac added inline comments.
web/apps/app-listing.react.js
45 ↗(On Diff #9892)

can we get these values out of CSS variables instead of hardcoding color values: https://davidwalsh.name/css-variables-javascript

49 ↗(On Diff #9892)

can we get these values out of CSS variables instead of hardcoding color values: https://davidwalsh.name/css-variables-javascript

web/apps/apps-directory.react.js
50 ↗(On Diff #9892)

nit: can we use markup? example: h3, p instead of div?

This is a header. matching element in typeography.css is

.h4,
h4 {
  font-size: var(--xl-font-20);
}
``
51 ↗(On Diff #9892)

same here

web/apps/apps.css
10 ↗(On Diff #9892)

can we use padding in the inner element instead of margin?

34 ↗(On Diff #9892)

same here. h5 is the same in typography.css`. I think we still need semi-bold.

This revision now requires changes to proceed.Feb 24 2022, 10:39 AM

Updated styles and html following Ben's comments

benschac added inline comments.
web/apps/apps.css
43–53 ↗(On Diff #9949)

This code is almost identical. Would it make sense to just have the font-size declared in appListingIconState?

tomek requested changes to this revision.Mar 1 2022, 5:26 AM
tomek added inline comments.
web/apps/app-listing.react.js
42 ↗(On Diff #9949)
59 ↗(On Diff #9949)

Just an idea: what do you think about adding a method toggleAppEnabled? This might simplify the code significantly. If you agree, we can do this as a next diff, as this one looks ready.

70 ↗(On Diff #9949)

Just a note: we should avoid using headings just to set text size. If the html document is properly formatted, it shouldn't skip any h level https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements

Avoid skipping heading levels: always start from <h1>, followed by <h2> and so on

The correct way to use these, is to use headings to mark the content, and CSS to set the size.

We can keep it for now though, as we're violating this rule in a lot of places.

web/apps/apps-directory.react.js
34–38 ↗(On Diff #9949)

I think we can improve the readability here

web/apps/apps.css
21 ↗(On Diff #9949)

We should use our theme

This revision now requires changes to proceed.Mar 1 2022, 5:26 AM

Updates following comments from review

This revision is now accepted and ready to land.Mar 4 2022, 10:21 AM
This revision was automatically updated to reflect the committed changes.