[Webhooks] Refactor Edit view #16800
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
feat(webhooks/EditView): remove the reducer feat(webhooks/EditView): disable save button until form is modified
jhoward1994
added
pr: enhancement
|
Size Change: +129 B (0%) Total Size: 1.5 MB
ℹ️ View Unchanged
|
gu-stav
reviewed
May 24, 2023
| return acc; | ||
| }, {}); | ||
|
|
||
| const StyledTable = styled.table` |
There was a problem hiding this comment.
Why do we need a custom table here?
There was a problem hiding this comment.
Do you think we should be using the Table or RawTable from the DS?
There was a problem hiding this comment.
If it is possible - yes definitely. In case something is missing we could check if we can add it to the DS. I'd say in the CMS consistency wins in a case like this.
There is also a Table component in the helper-plugin that already has even more defaults.
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
.../admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/HeadersInput/index.js
Outdated
Show resolved
Hide resolved
...e/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/WebhookForm/index.js
Show resolved
Hide resolved
...e/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/WebhookForm/index.js
Outdated
Show resolved
Hide resolved
packages/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/index.js
Outdated
Show resolved
Hide resolved
jhoward1994
added
the
flag: don't merge
joshuaellis
requested changes
May 24, 2023
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
...min/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/HeadersInput/Combobox.js
Outdated
Show resolved
Hide resolved
.../admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/HeadersInput/index.js
Outdated
Show resolved
Hide resolved
.../admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/WebhookForm/Layout/index.js
Outdated
Show resolved
Hide resolved
...admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/WebhookForm/Layout/Layout.js
Outdated
Show resolved
Hide resolved
...e/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/WebhookForm/index.js
Show resolved
Hide resolved
gu-stav
reviewed
Jun 1, 2023
packages/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/index.js
Outdated
Show resolved
Hide resolved
packages/core/content-type-builder/admin/src/components/FormModal/forms/index.js
Outdated
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
...in/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/HeadersInput/constants.js
Outdated
Show resolved
Hide resolved
jhoward1994
removed
the
flag: don't merge
gu-stav
reviewed
Jun 2, 2023
There was a problem hiding this comment.
Some QA feedback:
-
I'm not able to remove the last header. Is this on purpose? If so, IMO we should disable / hide (to check with Maeva) remove button. cc @maevalienard
-
Adding multiple headers: (This was caused by potentially duplicate keys for the header inputs)
- Click "Add new header" 3 times, but don't fill in values
- Click to the second to the last combobox and select a header
- Now 4 headers have been added and the combobox did not close
- Review workflow "stage change" events throw a validation error
...s/core/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/Events/index.js
Outdated
Show resolved
Hide resolved
.../admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/HeadersInput/index.js
Outdated
Show resolved
Hide resolved
.../admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/HeadersInput/index.js
Outdated
Show resolved
Hide resolved
...e/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/WebhookForm/index.js
Outdated
Show resolved
Hide resolved
...e/admin/admin/src/pages/SettingsPage/pages/Webhooks/EditView/components/WebhookForm/index.js
Outdated
Show resolved
Hide resolved
...in/src/pages/SettingsPage/pages/Webhooks/EditView/components/WebhookForm/tests/index.test.js
Outdated
Show resolved
Hide resolved
...in/src/pages/SettingsPage/pages/Webhooks/EditView/components/WebhookForm/tests/index.test.js
Outdated
Show resolved
Hide resolved
gu-stav
reviewed
Jun 2, 2023
Comment on lines
15
to
17
| jest.mock('../../../../../../../../hooks', () => ({ | ||
| useThemeToggle: jest.fn(() => ({ currentTheme: 'light', themes: { light: lightTheme } })), | ||
| })); |
There was a problem hiding this comment.
I believe you can also get rid of this, if you wrap the test below into ThemeProvider instead of ThemeToggleProvider?
There was a problem hiding this comment.
Do you still need the mock now that ThemeToggleProvider is gone?
gu-stav
reviewed
Jun 2, 2023
There was a problem hiding this comment.
More QA:
- When I change something in the edit-view, go to the list view and immediately back to the webook edit view the changes are not displayed; have to reload the page to see them.
Other than that I couldn't really find anything. I'm gonna use the opportunity to say: great work! I think this looks much better than before and it's great to see how easy it will become to extend the view. The pattern can probably be improved a bit further, but it's a great first step!
Comment on lines
15
to
17
| jest.mock('../../../../../../../../hooks', () => ({ | ||
| useThemeToggle: jest.fn(() => ({ currentTheme: 'light', themes: { light: lightTheme } })), | ||
| })); |
There was a problem hiding this comment.
Do you still need the mock now that ThemeToggleProvider is gone?
Comment on lines
+95
to
+99
| const getStoredPrivateAttributes = (model) => | ||
| union( | ||
| strapi?.config?.get('api.responses.privateAttributes', []) ?? [], | ||
| getOr([], 'options.privateAttributes', model) | ||
| ); |
There was a problem hiding this comment.
Not sure what happened here? Eslint?
| return ( | ||
| <> | ||
| <EventTable.Headers isDraftAndPublish={isDraftAndPublish} /> | ||
| <EventTable.Body isDraftAndPublish={isDraftAndPublish} extraEvents={events} /> |
There was a problem hiding this comment.
I think we can improve this further (maybe discuss it with Josh next week), but I think to get this merged it is ok for now.
joshuaellis
reviewed
Jun 5, 2023
Comment on lines
9
to
14
| renderChildren={({ isDraftAndPublish }) => ( | ||
| <> | ||
| <EventTable.Headers isDraftAndPublish={isDraftAndPublish} /> | ||
| <EventTable.Body isDraftAndPublish={isDraftAndPublish} /> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
This API is a bit odd for me imo, firstly, it'd be good to understand why we've opted for a render children pattern as opposed to using Context? Secondly, is there a reason children couldn't just be used as a function?
<EventTable.Root>
{(props) => <Children {...props} />}
</EventTable.Root>
joshuaellis
reviewed
Jun 5, 2023
...tingsPage/pages/Webhooks/EditView/components/WebhookForm/utils/getWebhookValidationSchema.js
Outdated
Show resolved
Hide resolved
joshuaellis
approved these changes
Jun 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does it do?
Refactor the web-hooks settings page edit view
Why is it needed?
To maintain the components and enable review workflow webhook events
BE review workflows webhooks implementation must be merged first