Skip to content
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

Refactor room edit modal #4408

Merged
merged 17 commits into from May 13, 2020
Merged

Conversation

plourenco
Copy link
Member

@plourenco plourenco commented Apr 15, 2020

Closes #3958

Update the room edit modal to a tabbed layout. Also refactor the form
into multiple sub-components from a single object.

Under heavy development and refactoring with a couple of TODOs:

  • Prevent the errors from showing when switching tabs (possibly with a form reset that keeps dirty)
  • Handle submit errors by showing the affected tab
  • Handle validation errors by showing the affected tab, or prevent switching if any error is triggered
  • This may include moving the validation into final-form.

image

indico/modules/rb/client/js/common/rooms/RoomEditModal.jsx Outdated Show resolved Hide resolved
indico/modules/rb/client/js/common/rooms/RoomEditModal.jsx Outdated Show resolved Hide resolved
indico/modules/rb/client/js/common/rooms/RoomEditModal.jsx Outdated Show resolved Hide resolved
indico/modules/rb/client/js/common/rooms/RoomEditModal.jsx Outdated Show resolved Hide resolved
</FormSpy>
<FormSpy subscription={{submitFailed: true}}>
Copy link
Member

@ThiefMaster ThiefMaster Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use a single FormSpy that subscribes to both submitSucceeded and submitFailed.

In principle you can probably avoid using FormSpy altogether and just add it to the global subscription in the FinalForm and then get these two values from the fprops.

Copy link
Member Author

@plourenco plourenco Apr 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had both approaches, ended up with this because using two FormSpy's will result in an extra empty <> to hold both Message components (unless we merge them and use conditionals).
Adding it as a global subscription would be perfect, but wouldn't that make the whole form re-render when we only need it in a small part?

@plourenco plourenco marked this pull request as ready for review Apr 21, 2020
Copy link
Member

@ParthS007 ParthS007 left a comment

@plourenco I have tested the edit modal and observed these 2 things:

Screenshot from 2020-04-22 10-53-38

When we upload a picture and click on save (button just below picture), It sends a POST and GET to show the image. After that, when we delete the picture just after that, it sends a DELETE and then GET to get the image with the same query string when we saved the image which gives a 404 on network tab. Is it supposed to work that way?

Screenshot from 2020-04-22 10-34-11
Now, we have validation error on 3 fields continuously, I can't select the Building field. Maybe the position of the tooltip can be changed?

Copy link
Member

@ParthS007 ParthS007 left a comment

why export default is not written with the function itself as there is only one function in most of the files?

indico/modules/rb/client/js/common/rooms/RoomEditModal.jsx Outdated Show resolved Hide resolved
Copy link
Member

@panagiotappl panagiotappl left a comment

Thanks for the PR! This task has been around for a while and it's a lot of messy work that needed to be improved!

I tested it locally and seems to run very smoothly! Along with the comment I mentioned yesterday in our MM channel about the existing photograph not showing the edit dialoge, I left a couple of smaller comments as well!

@plourenco
Copy link
Member Author

plourenco commented Apr 22, 2020

why export default is not written with the function itself as there is only one function in most of the files?

I think this was a personal preference. But I'm changing the style since the latter is more straightforward.

@plourenco
Copy link
Member Author

plourenco commented Apr 22, 2020

@plourenco I have tested the edit modal and observed these 2 things:

When we upload a picture and click on save (button just below picture), It sends a POST and GET to show the image. After that, when we delete the picture just after that, it sends a DELETE and then GET to get the image with the same query string when we saved the image which gives a 404 on network tab. Is it supposed to work that way?

Now, we have validation error on 3 fields continuously, I can't select the Building field. Maybe the position of the tooltip can be changed?

  1. Completely not expected. It was the simple order of setState updates in the async call that caused the files to be set to null before the preview flag was false.

  2. That was actually a big problem, probably affecting other fields. Nice catch!

It seems like we are using open together with trigger, however, the first one is preventing the latter.
Ideally, we don't need the Popups to be persistent, we only need to show them if there is an error and you actually want to find more information and click or hover on the input. (This is what the trigger controls).

Having the open set to showErrorPopup means it will either always stay closed if there is no error (expected) or always stay open if there is an error (not expected, the trigger should control this).

So what I did was not setting the open prop if there is an error.

open={showErrorPopup && undefined}

Copy link
Member

@ParthS007 ParthS007 left a comment

LGTM

indico/modules/rb/client/js/common/rooms/RoomEditModal.jsx Outdated Show resolved Hide resolved
@@ -7,952 +7,330 @@

Copy link
Member

@ThiefMaster ThiefMaster Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this file also go inside the edit/ subdir?

Copy link
Member Author

@plourenco plourenco Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, what do you think?
Since this is a core file, connected to the RoomEditDetails for me it made more sense to highlight it and move to the rooms.

Copy link
Member

@ThiefMaster ThiefMaster Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm you could always re-export it from edit/index.js so you can import RoomEditModal from './edit'; which I think is clearer (and that way anything for editing is in one place)

Copy link
Member Author

@plourenco plourenco Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if that is a better solution since the only thing I am importing and exporting is RoomEditModal itself, it is an unnecessary barrel. Nonetheless, I moved it into the edit.

<FormSpy subscription={{submitSucceeded: true}}>
{({submitSucceeded}) => (
<Button onClick={onClose}>
{submitSucceeded ? <Translate>Close</Translate> : <Translate>Cancel</Translate>}
Copy link
Member

@ThiefMaster ThiefMaster Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if that does what you intend: The old submitState was reset as soon as something changed in the form (so it went back from Close to Cancel if you started editing after saving) - I don't think this happens with submitSucceeded.

Copy link
Member Author

@plourenco plourenco Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the only difference I saw was not resetting after a successful submission.
However, do you think it is worth changing this to the internal state (another re-render) just because of a button change after the first submit?
It could also seem reasonable for the button to say Close if you already saved previously.

@plourenco plourenco self-assigned this Apr 28, 2020
mic4ael
mic4ael approved these changes May 5, 2020
@mic4ael
Copy link
Member

mic4ael commented May 5, 2020

Really great job! I am just thinking about one thing. Right now, when you change between tabs the height of the modal changes according to the tab's content. Wouldn't it be nicer if we could have (if possible) fixed height and prevent those changes?

@mic4ael
Copy link
Member

mic4ael commented May 5, 2020

Another thing that I just noticed. Not sure if we haven't noticed it so far, but when one uploads a new room photo, it is possible to upload for example a pdf. Backend of course will reject it, but as far as I know Dropzone also allows to define a list of allowed mimetypes.

@plourenco
Copy link
Member Author

plourenco commented May 6, 2020

Right now, when you change between tabs the height of the modal changes according to the tab's content. Wouldn't it be nicer if we could have (if possible) fixed height and prevent those changes?

I agree, it is annoying. Even though the modal is already top-aligned, I'm not sure it is possible to prevent it from stretching vertically besides adding a fixed height. We were previously using a scrollable modal, but I don't like that approach.

but as far as I know Dropzone also allows to define a list of allowed mimetypes.

This is a great suggestion, we can prevent this.

@plourenco plourenco force-pushed the tabbed-room-edit-modal branch 2 times, most recently from ecf75ca to a8e9fb9 Compare May 11, 2020
roomId: PropTypes.number,
locationId: PropTypes.number,
onClose: PropTypes.func.isRequired,
afterCreation: PropTypes.func,
Copy link
Member

@ThiefMaster ThiefMaster May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a bool?

plourenco and others added 15 commits May 13, 2020
Update the room edit modal to a tabbed layout. Also refactor the form
into multiple sub-components from a single object.
Create an error per tab logic that will highlight the corresponding
section.

Move the components part of the edit modal into an edit folder.
Form will now handle validation at field level.

Also, input names were changed to snake case.
Also adds missing validation to location and removes default
null fields.
Also removed the withKeyAttribute method and minor refactored
validators.
Replace the hideErrorPopup prop by the native semantic-ui trigger,
that shows or disables depending on the focus/hover state.

Create a field dependent validation for latitude and longitude.
Also, move the coordinates validation to the form level.
Also apply minor refactorings in the same component.
also reorder some imports
Even if they aren't submit errors. Also don't show the indicator for the
currently-active tab.
plourenco and others added 2 commits May 13, 2020
Also adds missing validation and moves the form rendering outside of a
function wrapper.
@ThiefMaster ThiefMaster merged commit e79fcd8 into indico:master May 13, 2020
5 checks passed
@ThiefMaster ThiefMaster deleted the tabbed-room-edit-modal branch May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants