Add interface preferences modal to edit post. #39176
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
8 tasks
|
Size Change: +6 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
tellthemachines
force-pushed
the
add/new-modal-to-edit-post
branch
3 times, most recently
from
343aea1 to
6c1532d
Compare
tellthemachines
force-pushed
the
add/new-modal-to-edit-post
branch
from
6c1532d to
ce36ab5
Compare
tellthemachines
commented
Mar 9, 2022
|
|
||
| // This allows us to tweak the returned value on each test. | ||
| jest.mock( '@wordpress/data/src/components/use-select', () => jest.fn() ); | ||
| jest.mock( '@wordpress/compose/src/hooks/use-viewport-match', () => jest.fn() ); | ||
|
|
||
| describe( 'PreferencesModal', () => { | ||
| describe( 'EditPostPreferencesModal', () => { |
There was a problem hiding this comment.
I'm not sure how useful these snapshots actually are as tests 🤷
There was a problem hiding this comment.
Yeah, I agree. We should be making explicit assertions rather than blindly comparing snapshots. Or maybe just remove them if they aren't helpful 🤷♂️.
There was a problem hiding this comment.
Same, I think there's a fair few component tests that are pretty much "Does it render" and used the snapshot approach to avoid having to write more detailed usability-focused tests. It'd be nice to follow-up at some point to add React Testing Library-based tests that more closely look at how the component's used (e.g. tabbing on larger viewports vs navigation in narrower ones, etc). For the moment, sticking with the snapshots sounds good to me, though, to avoid doing too much in the one PR.
There was a problem hiding this comment.
Yeah, I'm thinking the best way to test something like this might be e2e, but best look at that in another PR 😅
tellthemachines
added
[Package] Edit Post
kevin940726
approved these changes
Mar 9, 2022
| @@ -54,7 +54,7 @@ async function waitForPreviewNavigation( previewPage ) { | |||
| * @param {boolean} shouldBeChecked If true, turns the option on. If false, off. | |||
| */ | |||
| async function toggleCustomFieldsOption( shouldBeChecked ) { | |||
| const baseXPath = '//*[contains(@class, "edit-post-preferences-modal")]'; | |||
| const baseXPath = '//*[contains(@class, "interface-preferences-modal")]'; | |||
There was a problem hiding this comment.
Off-topic, just jogging my thoughts down: A good example of having class names in e2e selectors isn't a good idea. We could probably just do [role="dialog"] here, since there should be only one dialog/modal usable at any given time. It's not very readable what this modal is though, so an accessible role selector is still useful: role=dialog[name="Preferences"]
|
|
||
| // This allows us to tweak the returned value on each test. | ||
| jest.mock( '@wordpress/data/src/components/use-select', () => jest.fn() ); | ||
| jest.mock( '@wordpress/compose/src/hooks/use-viewport-match', () => jest.fn() ); | ||
|
|
||
| describe( 'PreferencesModal', () => { | ||
| describe( 'EditPostPreferencesModal', () => { |
There was a problem hiding this comment.
Yeah, I agree. We should be making explicit assertions rather than blindly comparing snapshots. Or maybe just remove them if they aren't helpful 🤷♂️.
andrewserong
reviewed
Mar 9, 2022
There was a problem hiding this comment.
This is testing well for me, too! Should we also remove the CSS rules that were duplicated in #39153 (e.g. around this line?)
Oooh well spotted! Done ✅ |
talldan
approved these changes
Mar 9, 2022
+1! Thanks for removing the extra CSS, too! ✨ |
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.
Description
Replaces the edit post preferences modal with the reusable one from the interface package added in #39153.
Testing Instructions
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).