digital-form-builder icon indicating copy to clipboard operation
digital-form-builder copied to clipboard

Edit summary behaviour (declaration/skip) from the page rather than the menu

Open JamesBoadi opened this issue 3 years ago • 5 comments

Description

Summary page behaviour previously was selected from the menu for skip summary and editing the declaration. Now skip summary has been deprecated and declaration can be edited from specifically clicking the summary page.

Type of change

Please delete options that are not relevant.

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

How Has This Been Tested?

Some unit tests need to be changed first since they use the summary page menu item

Checklist:

  • [ ] My changes do not introduce any new linting errors
  • [x] I have performed a self-review of my own code
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation and versioning
  • [ ] My changes generate no new warnings
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [ ] New and existing unit tests pass locally with my changes
  • [ ] I have rebased onto main and there are no code conflicts
  • [ ] I have checked deployments are working in all environments
  • [ ] I have updated the architecture diagrams as per Contribute.md

JamesBoadi avatar Feb 22 '22 09:02 JamesBoadi

p.s. do you mind sending over a video of this working on your branch? I've tried to pull but no dice

jenbutongit avatar Mar 23 '22 17:03 jenbutongit

@JamesBoadi - I've just converted this to a draft since I don't think you've got summary related behaviour in here. You can put it out of draft when it's ready and re request a review

jenbutongit avatar Mar 29 '22 14:03 jenbutongit

@JamesBoadi would it be possible to have a link to the issue in the description of this PR please 🙏

RichardBray avatar Apr 26 '22 17:04 RichardBray

@JamesBoadi would it be possible to have a link to the issue in the description of this PR please 🙏

#721

JamesBoadi avatar Apr 26 '22 19:04 JamesBoadi

I should add that when the title is changed while editing the page and click save Flyout does not close without calling this.props.onEdit (i've renamed this to this.props.closeFlyout) from the Page component, so the smoke test fails without this. As far as I know this function was added by someone else.

Currently onHide is being passed as props to Flyout, then the onHide function toggles the boolean that shows or hides the Flyout. I understand this might be an anti-pattern, since a class component is calling props from a functional component

I was thinking that additional values should be added to Flyout context so that it updates when the Flyout is toggled (e.g. user clicks save) then props to be updated can be passed alongside it to the onHide function, so this.props.closeFlyout won't need to be called onSubmit, maybe that could be tackled in a separate PR incase of breaking changes?

JamesBoadi avatar Apr 28 '22 12:04 JamesBoadi