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

Grid block refactor #105

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Grid block refactor #105

wants to merge 3 commits into from

Conversation

johngodley
Copy link
Member

@johngodley johngodley commented Jun 25, 2020

This is a big refactor that does several things:

  • It moves from span and offset to span and start - columns are absolutely positioned instead of being relative to the previous column. The user continues to see things in terms of offset
  • Columns now contain their own span and start attributes instead of storing everything in the grid container
  • Move to functional React with hooks instead of classes.

This results in many benefits:

  • Big reduction in code size and complexity. We no longer have to convert between start and offset all the time
  • CSS size is greatly reduced as we can apply the same CSS class to each column, instead of having to provide every possible combination on the parent
  • Possibility of using absolute positioning for future things (see Layout Grid: Allow overlapping of layers #63)
  • Should be easier to change how the grid resizing and snapping works

Other changes:

  • Column resizing is now squishy. That is, it will squish columns if you resize into them. This contrasts to the previous version where it would use spare-space, if available, but wouldn't squash. One effect is that if you squash a column down it doesn't expand back out if you change your mind - a problem?
  • It's no longer possible to move a column onto a different row. Previously, using the inspector only, you could modify the offset and span values by entering specific values that resulted in a column moving onto another row. I don't think anyone knew this was possible, and it wasn't very easy to use. Layout Grid: Allow overlapping of layers #63 and drag/drop columns could be a better way of changing columns, so I don't think the loss here is a problem

Upcoming changes:

  • Unit tests to check the grid continues to behave as expected
  • Block migration. The block attributes have changed, and we need to prevent invalidations. Ideally it can transparently accept old attributes and convert them to new ones when re-saved.
  • Further CSS size reduction. Maybe we can split out the IE prefix CSS into a separate IE only file?
  • Merge front.css and style.css together. I know we split them apart, but it makes integration difficult, and it would be better merged
  • Changing the layout auto-selects the first column. Not sure why, and it's annoying

Adds a global alignment to the grid container, and individual alignments to the child columns

# Conflicts:
#	blocks/layout-grid/editor.scss
#	blocks/layout-grid/src/grid/edit.js
#	blocks/layout-grid/style.scss
It’s 2020
Each column now stores the settings, not the parent container. This allows us to reduce the CSS size, and overall complexity
@johngodley
Copy link
Member Author

@jasmussen this is still a work in progress and doesn't immediately need any attention, but just putting it on your radar.

@jasmussen
Copy link
Member

This is a big one. I've received your ping, and it's on my ToDo, and I'll absolutely get to it. But I want to do it right.

@jasmussen
Copy link
Member

I tested this today, and I must say, the amount of red code here is impressive.

The block itself is also working really well. In fact, it seems to be working perfectly, except for a few issues that I believe are not related to this branch specifically, but rather the block itself. But perhaps simply due to testing this heavily, they appeared.

The bad part is, I can no longer reproduce the issues, either in master or this branch. But nevertheless, I did encounter an issue where I switched from 3 to 4 columns, then pressed Undo, and the layout was messed up rather badly. Behold these values:

Screenshot 2020-07-10 at 12 05 57

I don't know if I didn't properly clear cache, but I could not recreate the conditions to cause this. But it did involve undo/redo and switching the amount of columns.

But in every case, even when the column values got broken for inexplicable reasons, the CSS seemed completely correct, insofar as the frontend always matched the backend.

What is the next step to moving forward with this one? Do we need some Takashi testing? Did you need CSS input?

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.

2 participants