-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Front/webhooks editview #4962
Front/webhooks editview #4962
Conversation
…nt/webhooks-listview
…to front/webhooks-editview
…nt/webhooks-editview
padding: 54px 30px 30px; | ||
text-align: center; | ||
background-color: white; | ||
p, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you avoid global selectors? maybe you can create components instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't create a component just to set a line-height: normal
attribute. I wish it's a problem we will never have again after setting the new design system: this attribute should be set to all elements in a reset file, it's the way the entire admin should work.
For the <p/>
element, the selector :first-of-type
makes it non global.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, you are right. Regarding the reset css do you thing that this is a task we should undertake soon?
I know that we have an import of sanitize.css in the entry point of the admin do you think that we should remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a task we should indeed undertake soon, but it may take some time to check it doesn't broke the grid :(
sanitize.css
is great for some generic attributes but it seems we cannot custom it. Maybe we can replace it by our own reset rules (like button
hasn't an outline
, p
doesn't have a margin-bottom
by default, or all the ul
has a list-styleattribute set to
none`, etc) based on the future design system so we're sure not to specify it each time.
packages/strapi-admin/admin/src/containers/SettingsPage/index.js
Outdated
Show resolved
Hide resolved
packages/strapi-admin/admin/src/containers/Webhooks/ListView/index.js
Outdated
Show resolved
Hide resolved
packages/strapi-admin/admin/src/containers/Webhooks/ListView/index.js
Outdated
Show resolved
Hide resolved
packages/strapi-admin/admin/src/components/HeadersInput/index.js
Outdated
Show resolved
Hide resolved
packages/strapi-admin/admin/src/components/HeadersInput/index.js
Outdated
Show resolved
Hide resolved
packages/strapi-admin/admin/src/components/HeadersInput/index.js
Outdated
Show resolved
Hide resolved
packages/strapi-admin/admin/src/containers/Webhooks/ListView/index.js
Outdated
Show resolved
Hide resolved
packages/strapi-admin/admin/src/containers/Webhooks/ListView/index.js
Outdated
Show resolved
Hide resolved
packages/strapi-admin/admin/src/containers/Webhooks/ListView/index.js
Outdated
Show resolved
Hide resolved
packages/strapi-admin/admin/src/containers/Webhooks/ListView/index.js
Outdated
Show resolved
Hide resolved
packages/strapi-admin/admin/src/containers/Webhooks/EditView/index.js
Outdated
Show resolved
Hide resolved
actions: headersActions, | ||
}; | ||
|
||
const handleBlur = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you reorder your functions here as well?
packages/strapi-admin/admin/src/containers/Webhooks/EditView/utils/formatData.js
Outdated
Show resolved
Hide resolved
packages/strapi-admin/admin/src/containers/Webhooks/EditView/utils/formatData.js
Show resolved
Hide resolved
packages/strapi-admin/admin/src/containers/Webhooks/EditView/utils/schema.js
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## features/webhooks #4962 +/- ##
===================================================
Coverage ? 9.87%
===================================================
Files ? 635
Lines ? 8634
Branches ? 1334
===================================================
Hits ? 853
Misses ? 6553
Partials ? 1228
Continue to review full report at Codecov.
|
}); | ||
} | ||
} catch (err) { | ||
if (isMounted.current) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is needed here but the setShowModal(false)
should be inside the isMounted
condition
|
||
const schema = yup.object().shape({ | ||
name: yup | ||
.string(translatedErrors.string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string
property does not exist in this object. Here's the corresponding file
method: 'GET', | ||
}); | ||
|
||
if (isMounted.current) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed here
try { | ||
await schema.validate(modifiedData, { abortEarly: false }); | ||
|
||
if (isMounted.current) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isMounted logic here is not needed, we should use it when we are requesting the API
} | ||
} | ||
} catch (err) { | ||
if (isMounted.current) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Signed-off-by: Virginie Ky <virginie.ky@gmail.com>
Signed-off-by: Virginie Ky <virginie.ky@gmail.com>
onChange({ target: { name: inputName, value: Array.from(set) } }); | ||
}; | ||
|
||
console.log('yoyo'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the console?
Signed-off-by: Virginie Ky <virginie.ky@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description of what you did:
Webhook feature
My PR is a:
Main update on the:
Manual testing done on the following databases: