Page MenuHomePhabricator

[web] Change apps tab to be a modal

Authored by inka on Feb 21 2023, 3:46 AM.
Referenced Files
Unknown Object (File)
Thu, Mar 16, 2:23 AM
Unknown Object (File)
Mon, Mar 13, 8:06 AM
Unknown Object (File)
Mon, Mar 13, 12:04 AM
Unknown Object (File)
Thu, Mar 9, 12:46 PM
Unknown Object (File)
Thu, Mar 9, 11:11 AM
Unknown Object (File)
Thu, Mar 9, 11:01 AM
Unknown Object (File)
Wed, Mar 8, 9:50 AM
Unknown Object (File)
Wed, Mar 8, 12:50 AM



The Apps tab is changed to be a modal. Mind that the designs have changed a bit, see comments under the issue.

Screenshot 2023-02-21 at 12.40.54.png (668×1 px, 89 KB)

Test Plan

Run web app, pressed the "+" button in the top bar, checked that a modal appears. Check that the modal displays correclty and it is possible to add / remove calendar as an app (the modal works

Diff Detail

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

4 ↗(On Diff #22822)

This needs to be changed to

import { useSelector } from '../redux/redux-utils.js';

But this will break this file - in line 42 we rely on this being any-typed. So this will require more changes, thus I'll do this in a separate diff

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 21 2023, 3:54 AM
Harbormaster failed remote builds in B16689: Diff 22822!
inka requested review of this revision.Feb 21 2023, 3:57 AM
inka edited the summary of this revision. (Show Details)

Looks great, only one question:
Isn't the gap between the header Choose apps and actual content with apps bigger on design than on the screenshot you provide?

22 ↗(On Diff #22861)
47–57 ↗(On Diff #22861)

I find eg. padding-right: 20px; more readable but up to you

13 ↗(On Diff #22861)
25–27 ↗(On Diff #22861)

I can imagine the previous use of <a> has a purpose since it was changing the URL and redirecting the user to a different screen but in this case, it feels not appropriate to me. After clicking there is a certain action in the same location, so for me, it should be <button> (or to be precise our component <Button>).

However, I've seen both in the codebase so deferring to you or other reviewers.

Well the last design this is based on is not in figma, but a screen shot, so it's hard for me to tell what that space is exactly. But I maybe this is more similar:

image.png (668×1 px, 68 KB)

47–57 ↗(On Diff #22861)

This actually has a purpose, because both of these styles are applied to some items. So they are supposed to override one another. Doing it differently is possible, but not much better, so I'll leave it

25–27 ↗(On Diff #22861)

You're right. I'll change this in the diff that introduced this element: D6733

Address review, rebase. I also noticed that it's "Choose apps", not "Choose Apps", so fixing that as well

kamil added inline comments.
32 ↗(On Diff #22960)

I prefer this one but here it doesn't change anything so up to you

7 ↗(On Diff #22960)

Why align-items is specified twice?

47–57 ↗(On Diff #22861)

ahh correct, my oversight, sorry!

25–27 ↗(On Diff #22861)

I think this diff will be more appropriate for change, since in D6733 context <a> makes more sense, and now it feels not comprehensive but I don't want to be picky so let's leave this as it is 😄

Please check inline comments in D6733 before landing

This revision is now accepted and ready to land.Feb 24 2023, 2:04 AM
25–27 ↗(On Diff #22861)

You are right again, my bad. But I'll leave it sice the comments are already there. Thank you for checking D6733 !