Form to add an App Repository #1954
Conversation
| const password = "pass"; | ||
| const email = "foo@bar.com"; | ||
| const server = "docker.io"; | ||
| // wrapper.setState({ secretName, user, password, email, server, showSecretSubForm: true }); |
There was a problem hiding this comment.
Left-over from checking whether setState can work, or is this documenting the intention of the following code? (If so, how about preceding with
// The following 6 `wrapper` calls effectively the same as
// wrapper.setState({ secretName, user, password, email, server, showSecretSubForm: true });
or even just pulling the code out into a func setState() just in this module?
There was a problem hiding this comment.
oops, left-over yes, forgot to remove.
| padding: 0.6rem; | ||
| background-color: var(--clr-color-neutral-100); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm surprised we're needing so much customization of the styles? Can any of this be generalized across the app? (not now)
There was a problem hiding this comment.
Yes, this component was interesting. Since it was rendered in two places: the header as a result of the "Add repository button" and a table as a result of the "Edit" button, it inherited different style depending on its position (that's why this component is slightly larger, to define every style).
Regarding generalization, it's complex, we could move some things but we risk that we are missing some case in which the style change affects something unexpected (has happened to me before). It has to be very similar components to do so, for example, I have in my TODOs to generalize the state of headers (Applications, Catalog, App Repositories...) which are right now defined one by one.
| <Alert theme="danger"> | ||
| Found an error updating the repositories: {errors.update.message} | ||
| </Alert> | ||
| )} |
There was a problem hiding this comment.
It's not clear why this is being removed?
There was a problem hiding this comment.
The error was already being shown in the AppRepoForm so it was duplicated.
| @@ -21,6 +22,9 @@ const Modal = ({ | |||
| staticBackdrop, | |||
| }) => { | |||
| const onClose = () => onModalClose(); | |||
| // Control when users click outside | |||
| const ref = useRef(null); | |||
| useOutsideClick(onClose, [ref], showModal); | |||
There was a problem hiding this comment.
Not sure why we need to create a ref here, I guess it's tricky to document it with a test, but maybe useful link to more info, or just a bit more in the above comment?
There was a problem hiding this comment.
yep, I should have added a test. The problem is that usually a Modal gets closed when you click outside of it (at least that's how react-modal behaves). But in this case, it was not possible to be closed unless specifically implementing a button for it (each time you use it).
There was a problem hiding this comment.
actually this change is not needed, found out that there is a parameter for this: staticBackdrop. It was no clear what that was for.
Description of the change
AppRepositoryForm and AppRepoAddDockerCreds:
Possible drawbacks
Still need to debug a weird issue in which the docker-registry form causes a re-render of the AppList, closing the modal with all the form. Note that this issue is already present in the current UI.
Applicable issues