Table Block: Fixed width table cells on by default #49672
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
added
[Type] Enhancement
|
Size Change: +90 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e069084. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4651094999
|
t-hamano
force-pushed
the
update/table-default-fixed-cell
branch
from
e069084
to
a02bbc4
Compare
t-hamano
force-pushed
the
update/table-default-fixed-cell
branch
2 times, most recently
from
42ef1c7
to
88fca61
Compare
t-hamano
force-pushed
the
update/table-default-fixed-cell
branch
from
88fca61
to
f455ffe
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @aduth. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Thanks for putting this together @t-hamano 👍
It's coming together well so far but there are a couple of tweaks I think we need to make around the deprecation and fixtures.
Deprecation
✅ New deprecation attributes match correctly
✅ Save function matches
❓ Deprecation supports are missing the interactivity support
Fixtures
❓ The non-deprecation fixtures should reflect "default" attributes other than the use case they are target i.e. they shouldn't add hasFixedLayout: false but instead have their markup etc updated to reflect the latest.
❓ For the deprecation fixtures they should remain with the defaults they had at the time that block was current and migrate all the way to the latest i.e. they shouldn't have their hasFixedLayout set either.
❓ We should probably also be consistent between the block fixtures and the document fixtures i.e. rely on the default attribute as they both did originally.
test/integration/fixtures/blocks/core__table__deprecated-1.html
Outdated
Show resolved
Hide resolved
|
@aaronrobertshaw Thanks for the review! I made additional commits for:
If I don't understand correctly, please let me know 🙏 |
aaronrobertshaw
approved these changes
Mar 20, 2024
There was a problem hiding this comment.
Thanks for iterating on this @t-hamano 👍
If I don't understand correctly, please let me know
It looks like all the major stuff has been sorted. I think this is pretty much ready to go.
✅ New deprecation's supports match (interactivity support has been added)
✅ Default (current version) fixtures are correct
✅ New deprecation is covered by fixtures
✅ Consistency between snapshots and fixtures
✅ Fixture tests are passing
✅ Manually tests as advertised
LGTM 🚢
|
@t-hamano I've given this another run after the latest changes. LGTM 🚢 |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
* Table Block: Fixed width table cells on by default * Add interactivity support to deprecation * Revert some fixture changes * Update some fixtures * Simplify attributes * Revert "Simplify attributes" This reverts commit 679c662. * Extract cell query Co-authored-by: t-hamano <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: mtias <[email protected]>
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #16045
Alternative to #18870
What?
This PR turns on fixed table layout by default in the Table block.
Why?
I believe there is already consensus on this change, but I personally also think the default behavior of changing cell widths as text is entered is a bit confusing.
How?
With the addition of the deprecation, I have updated the fixture files, unit tests and e2e test snapshots.
Fixtures
Previous blocks with a default value of
falseforhasFixedLayoutwill have{hasFixedLayout: false}added as a comment delimiter by passing this deprecation. Therefore, I have added this comment delimiter to the existing fixture htmls.For the v4 deprecation I added this time, I added a test scenario as
core__table__deprecated-4.html.Unit Tests
The table block has two types of unit tests to ensure that data from the document is correctly pasted into the table:
I have updated these files so that the default value of
hasFixedLayoutistrue.E2E Tests
Added
has-fixed-layoutclass to the table element in the snapshot.