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

InstructionsWithWorkspace owns the resizer #32417

Merged
merged 7 commits into from Jan 6, 2020

Conversation

islemaster
Copy link
Contributor

@islemaster islemaster commented Dec 14, 2019

LP-838: As a first step towards simplifying our instructions resize code, moves ownership of the HeightResizer component up out of TopInstructions into the InstructionsWithWorkspace component.

I believe this makes more sense - TopInstructions and CodeWorkspace are both rendered by InstructionsWithWorkspace, both respecting the instructions size currently stored in redux. At the moment, TopInstructions also owns the resizer control that sits between these two resizable areas, but it's unclear why TopInstructions should be concerned with this resize bar, rather than simply rendering in the space it's given. (There are circumstances where content changes inside the instructions will trigger a container resize, but that seems like a side-effect, not a primary concern).

In the new setup, TopInstructions, HeightResizer and CodeWorkspace are siblings, and their parent InstructionsWithWorkspace owns the resize logic that governs these three components.

This is nearly a pure refactor - from a behavior point of view everything should be exactly the same. The structure of the DOM changes a bit, with the resizer moving out a few layers.

Testing story

My approach to this change:

  1. Pin existing resize behavior with an integration test on InstructionsWithWorkspace.
  2. Move HeightResizer, keeping tests passing.
  3. Do a little code cleanup to make things read nicely.

Also manual testing on CSF and non-CSF levels to sanity check that the resizer still looks and feels the same.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@islemaster islemaster changed the base branch from staging-next to staging December 18, 2019 01:15
@islemaster islemaster added code-hygiene refactor Better code, no behavior change labels Dec 19, 2019
@islemaster islemaster marked this pull request as ready for review December 19, 2019 20:20
Copy link
Contributor

@dmcavoy dmcavoy left a comment

Choose a reason for hiding this comment

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

LGTM. I like that this pulls HeightResizer out if TopInstructions.

I'm wondering if the next step after this is looking into the redux store which is managing instructions height and trying to simplify some of the complexity there.

@islemaster islemaster merged commit 66f4c29 into staging Jan 6, 2020
@islemaster islemaster deleted the instructions-resizer-moved branch January 6, 2020 20:35
@islemaster islemaster restored the instructions-resizer-moved branch January 15, 2020 18:49
@islemaster islemaster deleted the instructions-resizer-moved branch January 15, 2020 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-hygiene refactor Better code, no behavior change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants