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/option to create new form #149

Merged
merged 29 commits into from
Feb 9, 2021

Conversation

hannyle
Copy link
Contributor

@hannyle hannyle commented Feb 4, 2021

Description

Control buttons for Fill Form and Upload XML File are moved to the top
New button New form is added in Fill Form
Remove Hide button

Related issues

#135

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Changes Made

  • Remove Hide button and CardHeader in WizardAddObjectCard.js
  • Add CardHeader to WizardFillObjectDetailsForm.js, WizardUploadObjectXMLForm.js, WizardDraftObjectPicker.js
  • Move control buttons from bottom to top in CardHeader
  • Make CardHeader as sticky header
  • Add button + New form and logic to start a new empty form
  • Modify and update e2e test for creating a new form

Testing

  • e2e Tests

@hannyle hannyle self-assigned this Feb 4, 2021
@hannyle hannyle added the enhancement New feature or request label Feb 4, 2021
@hannyle hannyle added this to the MVP milestone Feb 4, 2021
@hannyle hannyle linked an issue Feb 4, 2021 that may be closed by this pull request
@hannyle hannyle removed this from the MVP milestone Feb 4, 2021
@blankdots
Copy link
Contributor

I think this needs to be rebased on #148 as it seems the changes there will affect it

@hannyle
Copy link
Contributor Author

hannyle commented Feb 5, 2021

Rebased on #148

Since now every submissionType has different control buttons, and the Hide button has been removed, I think we should do the focus when we enter skip-link differently for each submissionType.
For e.g.

  • focus on form input for Fill Form
  • focus on Choose file button for Upload XML File,
  • focus on first Continue button for Choose from drafts

But I understand it would be more complicated to do it like that, we can brainstorm for solutions.

@saulipurhonen
Copy link
Contributor

saulipurhonen commented Feb 5, 2021

From accessibility side of view there shouldn't be another scroll bar on object card.
I'd set overflow to visible for card, top margin to height of the app nav bar and remove the overflow: scroll from container.
@hannyle, do you think if there's any drawbacks if we remove the scroll from card container?

@saulipurhonen
Copy link
Contributor

Another behavior change is form validation:
Previous Submit <objectType> button click would trigger validation and display validation errors as:
Screenshot (17)

On this branch form validation is shown in snackbar error message.

I'd prefer the original way of presenting validation errors by form fields since this is more readable.

@hannyle
Copy link
Contributor Author

hannyle commented Feb 5, 2021

From accessibility side of view there shouldn't be another scroll bar on object card.
I'd set overflow to visible for card, top margin to height of the app nav bar and remove the overflow: scroll from container.
@hannyle, do you think if there's any drawbacks if we remove the scroll from card container?

I agree that having one scroll inside another scroll doesn't look good.
What do you. mean by top` margin to height of the app `nav bar

@saulipurhonen
Copy link
Contributor

What do you. mean by top` margin to height of the app `nav bar

If top margin of the card header is set to 0, it will stay behind the navigation bar when form is scrolled down.

@lilachic
Copy link
Contributor

lilachic commented Feb 5, 2021

Seems that saving an empty form happens with auto save and Save as Draft button.

Does the skip-link work here, I can't see focus moving?

@hannyle
Copy link
Contributor Author

hannyle commented Feb 5, 2021

Another behavior change is form validation:
Previous Submit <objectType> button click would trigger validation and display validation errors as:
Screenshot (17)

On this branch form validation is shown in snackbar error message.

I'd prefer the original way of presenting validation errors by form fields since this is more readable.

I fixed this one.
From what I understood this happened because Submit button is outside of the form now.

@hannyle hannyle closed this Feb 5, 2021
@hannyle hannyle reopened this Feb 5, 2021
@hannyle
Copy link
Contributor Author

hannyle commented Feb 5, 2021

Seems that saving an empty form happens with auto save and Save as Draft button.

Does the skip-link work here, I can't see focus moving?

Seems that saving an empty form happens with auto save and Save as Draft button.

  • Yes this is a bug, but not caused by this PR. Could you create an issue for this ? Thanks. OR I can create it later.

Does the skip-link work here, I can't see focus moving?

  • This PR has not fixed for the skip-link yet. I have some suggestions on the above comment.

@blankdots
Copy link
Contributor

blankdots commented Feb 5, 2021

I merged the #148 and seems that created conflicts 😞

@hannyle
Copy link
Contributor Author

hannyle commented Feb 5, 2021

From accessibility side of view there shouldn't be another scroll bar on object card.
I'd set overflow to visible for card, top margin to height of the app nav bar and remove the overflow: scroll from container.
@hannyle, do you think if there's any drawbacks if we remove the scroll from card container?

What do you. mean by top` margin to height of the app `nav bar

If top margin of the card header is set to 0, it will stay behind the navigation bar when form is scrolled down.

In the current code, the top of CardHeader is 0. I tried to remove overflow: scroll, but what we want is to have a sticky bar for form's control buttons. Could you give me code example of your suggestion ?

I got it fixed. Thanks :)

Copy link
Contributor

@saulipurhonen saulipurhonen 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!

@blankdots
Copy link
Contributor

@blankdots blankdots mentioned this pull request Feb 8, 2021
3 tasks
@saulipurhonen saulipurhonen merged commit ba2f850 into develop Feb 9, 2021
@saulipurhonen saulipurhonen deleted the feature/option-to-create-new-form branch February 9, 2021 09:51
@blankdots blankdots mentioned this pull request Feb 12, 2021
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.

Option to cancel editing a saved draft
4 participants