DS integration in Webhooks Listview #10732

ronronscelestes · 2021-08-13T15:12:58Z

What does it do?

Integration of DS components in Settings/Webhooks Listview 💅
Actions still work using actions logic that was already there - refacto after this PR
Old List component left in comments for future implementation of actions (as PopUpWarning)

Demo

Run this branch

strapi-cla · 2021-08-13T15:13:02Z

All committers have signed the CLA.

codecov · 2021-08-13T15:18:15Z

Codecov Report

Merging #10732 (5242244) into v4/ds-migration (f568ce0) will not change coverage.
The diff coverage is n/a.

❗ Current head 5242244 differs from pull request most recent head 9e5ff42. Consider uploading reports for the commit 9e5ff42 to get more accurate results

@@               Coverage Diff                @@
##           v4/ds-migration   #10732   +/-   ##
================================================
  Coverage            57.73%   57.73%           
================================================
  Files                  183      183           
  Lines                 6466     6466           
  Branches              1343     1343           
================================================
  Hits                  3733     3733           
  Misses                2263     2263           
  Partials               470      470

Flag	Coverage Δ
unit	`57.73% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e10c6a3...9e5ff42. Read the comment docs.

mfrachet

Overall, this looks great 😄 . I've put comments in the codebase, if something is not clear, feel free to reach out to me 😊 . Great job 👏🏻 🚀

mfrachet · 2021-08-16T06:57:56Z

packages/core/admin/admin/src/components/GlobalStyle/index.js

@@ -1,4 +1,3 @@
-import 'sanitize.css/sanitize.css';


🎉 🚀 💥

mfrachet · 2021-08-16T06:59:32Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

+        title={headerProps.title.label}
+        subtitle={headerProps.content}
+        primaryAction={(
+          <Button onClick={() => (canCreate ? handleGoTo('create') : {})} startIcon={<AddIcon />}>


In semantic HTML, a button is not a link. We should not use button to redirect to pages 😊

mfrachet · 2021-08-16T07:01:35Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

+                        webhooksToDelete.length > 0 && webhooksToDelete.length < webhooks.length
+                      }
+                      value={webhooksToDelete.length === webhooks.length}
+                      onValueChange={value => handleCheckboxChange('main', value)}


I would suggest to separate the logic between checking the "main" checkbox and checking the other ones. This will reduce the complexity of the handleCheckboxChange. Complexity, in this scenario, corresponds to the number paths existing in the "handleCheckboxChange".

And for clarity reasons, I would suggest to avoid the conditional in this function since it's arbitrary and note based on the data we have 😊

Something like:

const handleSelectAllCheckbox = () => {} const handleSelectOneCheckbox = () => {}

mfrachet · 2021-08-16T07:03:06Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

+                      <Row justifyContent="flex-end">
+                        {canUpdate && (
+                          <IconButton
+                            onClick={() => {
+                              handleGoTo(webhook.id);
+                            }}
+                            label="Edit"
+                            icon={<EditIcon />}
+                            noBorder
+                          />
+                        )}
+                        {canDelete && (
+                          <Box paddingLeft={1}>
+                            <IconButton
+                              onClick={() => handleDeleteClick(webhook.id)}
+                              label="Delete"
+                              icon={<DeleteIcon />}
+                              noBorder
+                            />
+                          </Box>
+                        )}
+                      </Row>


Instead of doing this, in v9 (or 10) of parts, you can use:

<Stack horizontal size={1}> <Button1/> <Button2/> </Stack>

and it will do the same as what you're trying to do 😊

mfrachet · 2021-08-16T07:05:19Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

-      <Header {...headerProps} />
-      {canRead && (
-        <div className="list-wrapper">
+      <HeaderLayout


This component should have an h1 (<HeaderLayout as="h1" />) on it and the page itself should have a <Main labelledBy="the-id-you-put-on-the-h1">

Hicham managed this the following way in the registration page 😊

Concerning the Main component => https://github.com/strapi/strapi/pull/10733/files#diff-5b8fde11bf3bfe8ba7a3b27f0c5e0d3838892702ff8f1d93ab0589f7351d53abR72

Concerning the title of the page => https://github.com/strapi/strapi/pull/10733/files#diff-5b8fde11bf3bfe8ba7a3b27f0c5e0d3838892702ff8f1d93ab0589f7351d53abR76

mfrachet · 2021-08-16T07:10:34Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

+              rowCount={webhooks.length}
+              footer={(
+                <TFooter onClick={() => (canCreate ? handleGoTo('create') : {})} icon={<AddIcon />}>
+                  Add another field to this collection type


I think this should go in a translation file 😊 . You have exampelp in this file using the formatMessage function

mfrachet · 2021-08-16T07:10:51Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

+                <Tr>
+                  <Th>
+                    <BaseCheckbox
+                      aria-label="Select all entries"


I think you should also put it in a translation file 😊

mfrachet · 2021-08-16T07:11:18Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

+                    />
+                  </Th>
+                  <Th>
+                    <TableLabel>name</TableLabel>


Same concerning all the Th content: I think they should go in a translation file 😛

mfrachet · 2021-08-16T07:12:06Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

+                    <Td>
+                      <Row>
+                        <Switch
+                          onLabel="Enabled"


I won't comment about it anymore about it, but every string occurence of a word, in plain english, existing in this file, should go in a translation file 😊

mfrachet · 2021-08-16T07:12:34Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

  );
 }

 export default ListView;
+
+// TO KEEP AS EXEMPLE UNTIL WE KNOW HOW TO DEAL WITH EVENTS AND FUNCTIONS DEFINED IN THIS COMPONENT


HichamELBSI · 2021-08-16T14:01:16Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

@@ -267,6 +200,36 @@ function ListView() {
    }
  };

+  const handleSelectAllCheckbox = () => {


In that function, you loop over webhooks 2 times.
The first time to extract ids. And the second time to select each webhook of the webhookIds.
I think you don't need to do that. You don't need to extract the ids of webhooks. Since you can access the state in the reducer function, you only need to create a function that select all the webhooks.

// Webhooks/ListView/index.js const handleSelectAllCheckbox = () => { dispatch({ type: 'SET_ALL_WEBHOOKS_TO_DELETE', }); };

// Webhooks/ListView/reducer.js ... case 'SET_ALL_WEBHOOKS_TO_DELETE': { const { webhooks, webhooksToDelete } = state; if (webhooks.length === webhooksToDelete.length) { draftState.webhooksToDelete = initialState.webhooksToDelete; } else { draftState.webhooksToDelete = webhooks.map(webhook => webhook.id); } break; } ...

Don't forget to add some test to this reducer case 😄

HichamELBSI · 2021-08-16T14:02:53Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

 import reducer, { initialState } from './reducer';
+import PageTitle from '../../../components/SettingsPageTitle';
+import adminPermissions from '../../../permissions';

 function ListView() {


You will need to call the focus hook when the i18n list view will be merged.

HichamELBSI · 2021-08-16T14:03:57Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

+
+        <ContentLayout>
+          <>
+            {rowsCount > 0 ? (


Something is wrong with the table, I can't focus the delete icon in each row and the last row also (with the keyboard).

You can focus on it if you are already focused on the edit icon then using tab. I talked about it with Marvin, if I understood well it's because it's a button/action so you have to use tab to access it 🕵️‍♀️

Oh thanks for the button, indeed it is better with the tab. But, I still can't reach the last list row with the keyboard 😢

(rows should include the header)

Oh right same here, I'm not sure why though! @mfrachet do you have an idea? 🤕

HichamELBSI · 2021-08-16T14:05:28Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

+            {rowsCount > 0 ? (
+              <Table
+                colCount={5}
+                rowCount={webhooks.length}


rowCount is already defined on the top of the file : rowCount={rowsCount}

HichamELBSI · 2021-08-16T14:05:55Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

+                          id: 'Settings.webhooks.list.all-entries.select',
+                        })}
+                        indeterminate={
+                          webhooksToDelete.length > 0 && webhooksToDelete.length < webhooks.length


Same : webhooksToDelete.length > 0 && webhooksToDelete.length < rowsCount

HichamELBSI · 2021-08-16T14:06:15Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

+                        indeterminate={
+                          webhooksToDelete.length > 0 && webhooksToDelete.length < webhooks.length
+                        }
+                        value={webhooksToDelete.length === webhooks.length}


Same : webhooksToDelete.length === rowsCount

HichamELBSI · 2021-08-16T14:10:26Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

+                          id={`${formatMessage({ id: 'Settings.webhooks.list.select' })} ${
+                            webhook.id
+                          }`}
+                          name={`${formatMessage({ id: 'Settings.webhooks.list.select' })} ${
+                            webhook.name
+                          }`}


The id and name aren't used to be understood by the user. We should use static names and ids not translated messages.

mfrachet

I've put a request for changes to investigate the a11y way to handle this page loading :)

mfrachet · 2021-08-18T09:37:35Z

packages/core/admin/admin/src/pages/Webhooks/ListView/index.js

-              linkText={formatMessage({ id: 'Settings.webhooks.list.empty.link' })}
+      <Main labelledBy="webhooks">
+        {isLoading ? (
+          <LoadingIndicatorPage />


Even when displaying the loader, I think the HeaderLayout should be on the page because else, during this tiny moment where nothing is displayed, there's nothing on the screen and the page is in an invalid state because there's not H1 on it.

Also, when loading something, you can put a aria-busy={isLoading} on the "Main" component so that it tells people using ATs that something is happening.

Later on, when the data has been loader, I think it's a common practice to use an aria region to announce that the content has been loaded and that the content of the Table is ready to be navigated 😊

HichamELBSI

Good job !!

ronronscelestes added 2 commits August 13, 2021 12:38

integrated DS + deleteOne, updateOne

73a03fb

Switch && Checkboxes actions + empty webhooks ds

0254876

ronronscelestes requested review from HichamELBSI and mfrachet August 13, 2021 15:13

ronronscelestes changed the title ~~Integration of DS in Webhooks Listview~~ DS integration in Webhooks Listview Aug 13, 2021

$mfrachet$

mfrachet suggested changes Aug 16, 2021

View reviewed changes

ronronscelestes added 2 commits August 16, 2021 12:00

PR review fixes

bb902a3

small PR review fix

c270560

HichamELBSI requested changes Aug 16, 2021

View reviewed changes

ronronscelestes added 8 commits August 17, 2021 10:39

adding a reducer + test

834e0e9

set all webhooks to delete reducer created with tests works

5b3113a

more PR review fixes

a94929d

deleted a comment

5ce9d2c

Merge branch 'v4/ds-migration' into webhooks-listview-ds

f79f6ed

added useFocusWhenNavigate hooks

b944482

added 1 to rowsCount in Table to able focus on last row

975b1e9

Merge branch 'v4/ds-migration' into webhooks-listview-ds

e8c352e

$mfrachet$

mfrachet suggested changes Aug 18, 2021

View reviewed changes

ronronscelestes added 2 commits August 18, 2021 11:49

changed enabled - disabled format message

6a609cc

loading state updated

81ae331

HichamELBSI approved these changes Aug 18, 2021

View reviewed changes

ronronscelestes added 3 commits August 19, 2021 12:00

updated to parts alpha 15 + LinkButton

5242244

add webhook link button if canCreate + notifyStatus

9e5ff42

fix conflicts

c987be0

soupette merged commit de87cb8 into v4/ds-migration Aug 19, 2021

soupette deleted the webhooks-listview-ds branch August 19, 2021 13:42

Bhanditz mentioned this pull request Nov 26, 2023

[Snyk] Fix for 16 vulnerabilities Bhanditz/strapi#146

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DS integration in Webhooks Listview #10732

DS integration in Webhooks Listview #10732

ronronscelestes commented Aug 13, 2021

strapi-cla commented Aug 13, 2021 •

edited

codecov bot commented Aug 13, 2021 •

edited

$@mfrachet$ mfrachet left a comment

$@mfrachet$ mfrachet Aug 16, 2021

$@mfrachet$ mfrachet Aug 16, 2021

$@mfrachet$ mfrachet Aug 16, 2021

$@mfrachet$ mfrachet Aug 16, 2021

$@mfrachet$ mfrachet Aug 16, 2021

$@mfrachet$ mfrachet Aug 16, 2021

$@mfrachet$ mfrachet Aug 16, 2021

$@mfrachet$ mfrachet Aug 16, 2021

$@mfrachet$ mfrachet Aug 16, 2021

$@mfrachet$ mfrachet Aug 16, 2021

HichamELBSI Aug 16, 2021

HichamELBSI Aug 16, 2021

HichamELBSI Aug 16, 2021

HichamELBSI Aug 16, 2021

ronronscelestes Aug 17, 2021

HichamELBSI Aug 17, 2021 •

edited

$@mfrachet$ mfrachet Aug 17, 2021

ronronscelestes Aug 17, 2021

ronronscelestes Aug 17, 2021

HichamELBSI Aug 16, 2021

HichamELBSI Aug 16, 2021

HichamELBSI Aug 16, 2021

HichamELBSI Aug 16, 2021

$@mfrachet$ mfrachet left a comment

$@mfrachet$ mfrachet Aug 18, 2021

HichamELBSI left a comment

DS integration in Webhooks Listview #10732

DS integration in Webhooks Listview #10732

Conversation

ronronscelestes commented Aug 13, 2021

What does it do?

Demo

strapi-cla commented Aug 13, 2021 • edited

codecov bot commented Aug 13, 2021 • edited

Codecov Report

mfrachet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HichamELBSI Aug 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfrachet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HichamELBSI left a comment

Choose a reason for hiding this comment

strapi-cla commented Aug 13, 2021 •

edited

codecov bot commented Aug 13, 2021 •

edited

$@mfrachet$ mfrachet left a comment

HichamELBSI Aug 17, 2021 •

edited

$@mfrachet$ mfrachet left a comment