Introduce Apps panel similar to existing in native app.
FontAwesome icons will soon be replaced by SWMIcons in follow-up diff.
Details
- Reviewers
tomek • benschac atul - Commits
- rCOMM1db6f07a77f2: [web] Introduce Apps page
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
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? | |
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. |
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 |
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. |
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? |
web/apps/app-listing.react.js | ||
---|---|---|
42 ↗ | (On Diff #9949) | Should we include these in https://linear.app/comm/issue/ENG-700/replace-fontawesome-icons-with-swmiconpack? |
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
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 |