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

Feature/enable draft update #528

Merged
merged 4 commits into from
Nov 10, 2021
Merged

Conversation

saulipurhonen
Copy link
Contributor

@saulipurhonen saulipurhonen commented Nov 5, 2021

Description

Added option for user to update saved templates.

Related issues

Closes #488

Type of change

  • New feature (non-breaking change which adds functionality)

Changes Made

  • Template menu action for edit functionality
  • Simplified templates list rendering (previous version caused nested tags with form dialog)
  • Option for rendering WizardFillObjectDetailsForm component in a dialog
  • Form actions for template -typed object
  • Reset draftStatus and currentObject redux state objects on form component unmount
  • Handle template replace both into backend and redux state
  • Prevent re-render of app by moving status message handling rendering logic to message handler from app root
  • Serialize status message details
  • Updated status message handler tests
  • Added test for template edit

Testing

  • Integration Tests

@saulipurhonen
Copy link
Contributor Author

Changes to status message handling had to be made since re-rendering whole app when status message state changes would unmount certain elements, eg. dialogs.

@saulipurhonen saulipurhonen force-pushed the feature/enable-draft-update branch from 1cbb517 to be89b97 Compare November 5, 2021 15:34
Status message handler changes

Prevent status message handler from re-rendering component tree
@saulipurhonen saulipurhonen force-pushed the feature/enable-draft-update branch from be89b97 to e7a3c4c Compare November 7, 2021 12:43
@saulipurhonen saulipurhonen marked this pull request as ready for review November 7, 2021 13:02
@saulipurhonen saulipurhonen self-assigned this Nov 7, 2021
@saulipurhonen saulipurhonen added the enhancement New feature or request label Nov 7, 2021
@saulipurhonen saulipurhonen added this to the Open Beta milestone Nov 7, 2021
@saulipurhonen saulipurhonen linked an issue Nov 7, 2021 that may be closed by this pull request
Copy link
Contributor

@hannyle hannyle left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@hannyle
Copy link
Contributor

hannyle commented Nov 9, 2021

I've actually encountered this error from Console when updating a template

Screenshot 2021-11-09 at 11 03 13

@saulipurhonen saulipurhonen force-pushed the feature/enable-draft-update branch from e7a3c4c to 7c47bd6 Compare November 9, 2021 11:09
@saulipurhonen
Copy link
Contributor Author

I've actually encountered this error from Console when updating a template

Good call. This error was caused because I typed response as optional property for StatusDetails type. Actions need to be serialized and this caused problems when response has nested unserialized data.

Fixed by stringifying response.

Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

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

image

Looks good, just a few notes:

  • could be a tad wider
  • isn't clear form + update the same as delete. my question would be what would be the use case for clear form ? - we could also omit it

@saulipurhonen
Copy link
Contributor Author

  • could be a tad wider
  • isn't clear form + update the same as delete. my question would be what would be the use case for clear form ? - we could also omit it

Good points. I think we could change the width once we're doing whole design overhaul.

Not sure if "Clear form" button is needed. Could there be some use cases where user would want to use the template id but re-fill the form?

May be we could prevent updating empty form?

@blankdots
Copy link
Contributor

Good points. I think we could change the width once we're doing whole design overhaul.

ok

Not sure if "Clear form" button is needed. Could there be some use cases where user would want to use the template id but re-fill the form?

May be we could prevent updating empty form?

let us remove clear form, I don't see a use case for it, and if we find one we will add it back
And yes we should prevent empty form, it there is the case, but do you mean prevent in front-end or back-end ?

@saulipurhonen
Copy link
Contributor Author

let us remove clear form, I don't see a use case for it, and if we find one we will add it back And yes we should prevent empty form, it there is the case, but do you mean prevent in front-end or back-end ?

I mean preventing empty form in front-end which is in the matter of fact implemented already.

@saulipurhonen saulipurhonen force-pushed the feature/enable-draft-update branch from 7c47bd6 to bcde668 Compare November 10, 2021 11:58
@saulipurhonen saulipurhonen merged commit 02d193e into develop Nov 10, 2021
@saulipurhonen saulipurhonen deleted the feature/enable-draft-update branch November 10, 2021 12:47
@blankdots blankdots mentioned this pull request Apr 7, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Users can Update templates
3 participants