Add List view for apps as default #690

bharatkashyap · 2022-07-25T18:04:16Z

I have followed (at least) the PR section of the contributing guide.
Added a list view for apps with a toggle button to switch to grid,
- Possible extension to allow collapsible rows for folders
Removed the "Apps" heading in favour of a Tabs list in the AppBar,
- Possible extension to allow a global "Data" or "Connections" tab
Rewrote the EditableText component to use only TextFields and leak minimal implementation details
Added a AppNameEditable component (along with some other to allow reuse between Card and Row) to store app name specific renaming logic
Remove tooltips from app names
Fixed a typo in getting readable duration
Fixes Renaming apps breaks styling #689

Screen.Recording.2022-07-31.at.11.40.47.PM.mov

render · 2022-07-25T18:04:22Z

Your Render PR Server URL is https://toolpad-pr-690.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-cbfdm90nlki93ndc64rg.

packages/toolpad-app/src/components/EditableText.tsx

Janpot · 2022-07-27T10:24:14Z

I believe one of the goals of this component should be to hide away the implementation details of how we switch between editable/non-editable state. This PR seems to leak more implementation details than before. In the ideal scenario, consumers of this component don't know whether it's built by switching between typography and textfield, or by styling a single input. I suggest we base the styling purely on variant and sx props for both non-editable and editable state. If that's too hard we can perhaps compromise on sx only as a start.

In case it could be helpful, typograpy styles are aviable in the theme object under theme.typography[variant]

render · 2022-07-29T16:04:35Z

Your Render PR Server URL is https://toolpad-pr-690.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-cbi0a4di0e896j46s7qg.

packages/toolpad-app/src/components/EditableText.tsx

Janpot · 2022-08-01T06:26:23Z

packages/toolpad-app/src/components/EditableText.tsx

  errorText?: string;
-  onKeyUp: (event: React.KeyboardEvent<HTMLInputElement>) => void;
-  onBlur: (event: React.FocusEvent<HTMLInputElement>) => void;
+  onKeyUp?: (event: React.KeyboardEvent<HTMLInputElement>) => void;


I'm not super fond of leaking these key handlers. Would it be make sense to build this using controlled properties only? i.e. value and onChange that act like a regular TextField. And to change editable state, open and onClose, analog to e.g. the MUI Dialog, Menu, Drawer,...

Janpot · 2022-08-01T06:33:39Z

test/e2e-smoke/basic.spec.ts

@@ -19,9 +19,13 @@ test('basic app creation flow', async ({ page }) => {

  await page.click('[aria-label="Home"]');

-  const appCardSelector = `[role="article"]:has-text("${appName}")`;
+  const appRow = await page.locator(`[role="row"]`, {
+    has: await page.locator(`input[value="${appName}"]`),


I'd prefer if we could keep the old locator. I have a hunch that there's an accessibility issue here. What happens if we add the aria-readonly to the input when it's not editable?

Adding the aria-readonly still doesn't help use the original selector, which relied on a has-text; but now since the name resides in the value of an input, has-text fails to find the element.

I've modified it a bit to use the selector format instead of locators, and now seems like the tests pass for Firefox as well

Janpot · 2022-08-01T06:34:25Z

@bharatkashyap It seems like the title and description of this PR are a bit off?

bharatkashyap · 2022-08-01T07:01:24Z

@bharatkashyap It seems like the title and description of this PR are a bit off?

The description explains some of the changes done to add the List view

packages/toolpad-app/src/components/EditableText.tsx

Janpot · 2022-08-05T07:32:34Z

packages/toolpad-app/src/toolpad/Home.tsx

+  value: number;
+}
+
+function TabPanel(props: TabPanelProps) {


In other places we use the MUI TabContext and TabPanel.

Janpot · 2022-08-05T07:35:36Z

packages/toolpad-app/src/toolpad/Home.tsx

-          })()}
-        </Box>
-      </Container>
+      <TabPanel index={0} value={tabIndex}>


Does this have to be part of this PR? I'm not quite sure what it's for

This is to move the "Apps" heading into an "Apps" tab; it could potentially be moved to a separate PR but feels like it is close enough feature wise to be part of the Home UI revamp

Is there a need for this right now? It seems a bit redundant. Also, it looks (and works) completely different from how this menu is in the editor. we should probably aim for more consistency (there's also breadcrumbs in the release workflow). Maybe let's leave it out of the scope of this PR?

packages/toolpad-app/src/toolpad/Home.tsx

Janpot · 2022-08-05T07:40:34Z

packages/toolpad-app/src/toolpad/Home.tsx

+            </ToggleButtonGroup>
+          </Toolbar>
+          {viewMode === 'grid' ? (
+            <Box


Would it make sense to extract a AppsGridView and a AppsTableView component, with the exact same interface and switch between them bases on viewMode? e.g.

const AppsView = viewMode === 'grid' ? AppsGridView : AppsTableView // ... <AppsView apps={...} />

Signed-off-by: Jan Potoms <[email protected]>

Janpot · 2022-08-10T10:36:57Z

Resolved the conflicts that emerged after merging #723

Janpot

👌

Make inputProps controllable

823794f

oliviertassinari requested a deployment to app-card-rename - toolpad-db PR #690 July 25, 2022 18:04 — with Render Abandoned

bharatkashyap requested review from bytasv and Janpot July 27, 2022 09:15

bytasv reviewed Jul 27, 2022

View reviewed changes

packages/toolpad-app/src/components/EditableText.tsx Outdated Show resolved Hide resolved

Use only TextField for better abstraction

0a667bf

oliviertassinari requested a deployment to app-card-rename - toolpad-db PR #690 July 29, 2022 16:04 — with Render Abandoned

bharatkashyap changed the title ~~Make inputProps controllable~~ Rewrite EditableText for a tighter abstraction Jul 29, 2022

bharatkashyap added 8 commits July 30, 2022 00:29

Add Tabs and ToggleButtonGroup

d7b96fd

Toggle ToggleButton colour

1c632b8

Add apps list table

bff3b48

Merge branch 'app-card-rename' into home-ui

5fbabc3

Use EditableText for apps list, disableTooltip

b45f0c0

Further purify EditableText

315d949

Reuse components for list, grid

6c1180f

Fix hydration issues due to TableRow not rendering

9032375

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 31, 2022

bharatkashyap changed the title ~~Rewrite EditableText for a tighter abstraction~~ Add List view for apps as default Jul 31, 2022

bharatkashyap added 3 commits July 31, 2022 14:26

Merge branch 'master' of github.com:mui/mui-toolpad into app-card-rename

44fa3cd

Modify smoke test

42700d3

Smoke tests, remove typo in duration text

f7f00df

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 31, 2022

bharatkashyap added 3 commits July 31, 2022 16:00

Pass inputProps to underlying input

af88666

Disallow click to edit for App name

420ece5

modify smoke tests for list view

bdd5959

Janpot reviewed Aug 1, 2022

View reviewed changes

bharatkashyap added 3 commits August 1, 2022 14:48

Make comment more explanatory

f37f824

Leaner interface for EditableText

c969b93

Smoke tests closer to original

43b136e

Janpot reviewed Aug 2, 2022

View reviewed changes

packages/toolpad-app/src/components/EditableText.tsx Outdated Show resolved Hide resolved

Janpot reviewed Aug 2, 2022

View reviewed changes

packages/toolpad-app/src/components/EditableText.tsx Outdated Show resolved Hide resolved

bharatkashyap added 2 commits August 3, 2022 02:43

Abstract disabling styles

5f063cc

Use standard adjective form for state prop

2c28c6d

bharatkashyap requested a review from Janpot August 4, 2022 11:52

Janpot reviewed Aug 5, 2022

View reviewed changes

packages/toolpad-app/src/toolpad/Home.tsx Outdated Show resolved Hide resolved

Janpot reviewed Aug 5, 2022

View reviewed changes

bharatkashyap added 3 commits August 5, 2022 19:27

Use TabContext, create common AppView component

2278014

Fix naming

9970d94

Remove Tabs from the scope of this PR

4f14e24

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 10, 2022

Merge branch 'master' into app-card-rename

5ceff71

Signed-off-by: Jan Potoms <[email protected]>

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 10, 2022

Janpot approved these changes Aug 16, 2022

View reviewed changes

bharatkashyap merged commit 4a05e24 into master Aug 17, 2022

bharatkashyap deleted the app-card-rename branch August 17, 2022 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add List view for apps as default #690

Add List view for apps as default #690

bharatkashyap commented Jul 25, 2022 •

edited

Loading

render bot commented Jul 25, 2022

Janpot commented Jul 27, 2022 •

edited

Loading

render bot commented Jul 29, 2022

Janpot Aug 1, 2022

Janpot Aug 1, 2022

bharatkashyap Aug 1, 2022

Janpot commented Aug 1, 2022

bharatkashyap commented Aug 1, 2022

Janpot Aug 5, 2022

Janpot Aug 5, 2022 •

edited

Loading

bharatkashyap Aug 5, 2022 •

edited

Loading

Janpot Aug 5, 2022 •

edited

Loading

Janpot Aug 5, 2022

Janpot commented Aug 10, 2022

Janpot left a comment

Add List view for apps as default #690

Add List view for apps as default #690

Conversation

bharatkashyap commented Jul 25, 2022 • edited Loading

render bot commented Jul 25, 2022

Janpot commented Jul 27, 2022 • edited Loading

render bot commented Jul 29, 2022

Janpot Aug 1, 2022

Choose a reason for hiding this comment

Janpot Aug 1, 2022

Choose a reason for hiding this comment

bharatkashyap Aug 1, 2022

Choose a reason for hiding this comment

Janpot commented Aug 1, 2022

bharatkashyap commented Aug 1, 2022

Janpot Aug 5, 2022

Choose a reason for hiding this comment

Janpot Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

bharatkashyap Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Janpot Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Janpot Aug 5, 2022

Choose a reason for hiding this comment

Janpot commented Aug 10, 2022

Janpot left a comment

Choose a reason for hiding this comment

bharatkashyap commented Jul 25, 2022 •

edited

Loading

Janpot commented Jul 27, 2022 •

edited

Loading

Janpot Aug 5, 2022 •

edited

Loading

bharatkashyap Aug 5, 2022 •

edited

Loading

Janpot Aug 5, 2022 •

edited

Loading