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

Keep the overview open when the article form closes #770

Merged
merged 3 commits into from Jun 5, 2019

Conversation

jonathonherbert
Copy link
Contributor

What's changed?

A side effect of #749 (that I added) changed the behaviour of the form toggles -- opening a form would close the overview for the front. This isn't desirable re: feedback from Mariana, so this PR reverts that behaviour: opening a form doesn't interfere with the overview state.

Checklist

General

  • πŸ€– Relevant tests added
  • βœ… CI checks / tests run locally
  • πŸ” Checked on CODE

Client

  • 🚫 No obvious console errors on the client (i.e. React dev mode errors)
  • πŸŽ›οΈ No regressions with existing user interactions (i.e. all existing buttons, inputs etc. work)
  • πŸ“· Screenshots / GIFs of relevant UI changes included

@RichieAHB
Copy link
Contributor

So does the overview actually stay visible here too? My assumption here was that we'd want to hide the overview while the panel was open but it reappears when cancel was clicked (or more generally, that the editor is closed). Mariana did mention the cancel button in the email ... you may already have discussed this and now that I think about it, this solution is probably better. I just wondered whether this is definitely the required behaviour?

@jonathonherbert
Copy link
Contributor Author

jonathonherbert commented Jun 5, 2019

@RichieAHB -- no, the overview is hidden by the form when the form is present -- it's basically the old behaviour.

@@ -73,7 +73,8 @@ const FormContainer = styled(ContentContainer.withComponent('form'))`
min-width: ${formMinWidth}px;
max-width: 380px;
margin-left: 10px;
height: 100%;
margin-top: 10px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A sneaky spacing edit to stop the form abutting the fronts header.

@RichieAHB
Copy link
Contributor

@jonathonherbert - I get it now. I thought the closing of the overview was the only thing stopping the overview rendering but I see here that there is already a check to ensure we don't render the overview if we have a form open πŸ‘

Copy link
Contributor

@RichieAHB RichieAHB left a comment

Choose a reason for hiding this comment

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

LGTM πŸ‘

@jonathonherbert
Copy link
Contributor Author

@RichieAHB -- ah, yes, I could have made that clearer!

@jonathonherbert jonathonherbert merged commit b68bde8 into master Jun 5, 2019
@jonathonherbert jonathonherbert deleted the jsh/keep-overview-open branch June 5, 2019 12:03
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.

None yet

2 participants