PaletteEdit: add changelog #46095
Merged
PaletteEdit: add changelog #46095
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
|
|
|
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
ramonjd
force-pushed
the
add/tests-and-changelog-for-palette-edit
branch
from
428bc8e to
d5bdac6
Compare
ramonjd
added
[Type] Code Quality
andrewserong
reviewed
Dec 2, 2022
There was a problem hiding this comment.
Thanks for the follow-up PR! Just left a couple of nits, and a question about whether we can use the existing labels within each component as targets for the test, instead of adding an additional aria-label value at the wrapper level, since it sounds like aria-label is intended to be used on interactive elements rather than wrappers, as far as I can tell.
Let me know if I'm way off, though!
ramonjd
force-pushed
the
add/tests-and-changelog-for-palette-edit
branch
from
d5bdac6 to
03b2045
Compare
andrewserong
reviewed
Dec 2, 2022
| slugPrefix: '', | ||
| }; | ||
|
|
||
| it( 'opens color selector for color palettes', async () => { |
There was a problem hiding this comment.
Thanks for updating so quickly!
The tests are passing for me locally, but I noticed the Github Action appears to be failing. I kicked it off again, but if it still fails, would it be worth trying to remove the async here? I wasn't sure if it's needed since the other test doesn't have it 🤔
There was a problem hiding this comment.
That was an oversight from previous testing. Thanks again for 👀
There was a problem hiding this comment.
Hrm, for some reason the test is still complaining 🤔
Happy to help debug on Monday!
There was a problem hiding this comment.
Yeah it's curious. fireEvent is already wrapped in act() apparently.
Tried using async getBy* queries as well. Still failed. 🤷
Oh well.
ramonjd
force-pushed
the
add/tests-and-changelog-for-palette-edit
branch
from
835e45a to
902a48b
Compare
|
If this round of testing doesn't work I think we should separate the requirement for a changelog for #45681 and the tests. The fact that it only fails on CI is troubling. 😄 |
Good idea, thanks for persevering! |
|
It appears to me to be related to the Popover component, which is a child of the PaletteEdit component. The Popover component's tests require the use of async testing code, but we're not taking that into account here. The only thing I can think of is to mock the component somehow 🤷 |
Oh, good catch. Would it work to use that wrapper ( |
ramonjd
changed the title
Tried it 😄 I think the latter since we're changing props sent to the Popover, which we're then querying. Strange that the color test passed, but not the gradient 🤷 I'll look into it more deeply in a follow up. Thanks for reviewing again. |
andrewserong
approved these changes
Dec 5, 2022
ramonjd
force-pushed
the
add/tests-and-changelog-for-palette-edit
branch
from
65cf20a to
57c786c
Compare
|
I readded the basic test to get some coverage since it was passing fine. |
- Add changelog for changes merged in #45681 - Test case for Popover
Relying on existing control aria labels to test for the presence of gradient and color pickers rather than the container.
…failing (not local)
ramonjd
force-pushed
the
add/tests-and-changelog-for-palette-edit
branch
from
57c786c to
170325c
Compare
Good thinking 👍 |
mpkelly
pushed a commit
to mpkelly/gutenberg
that referenced
this pull request
Dec 7, 2022
* Initial commit: - Add changelog for changes merged in WordPress#45681 - Test case for Popover * Change log entry * Formatting * Changelog formatting Relying on existing control aria labels to test for the presence of gradient and color pickers rather than the container. * Remove unnecessary async function dec * Export no longer required * Testing async queries using findByLabelText because the CI tests are failing (not local) * Let's try this. * Let's try this. * What about this? * Why not * And so it continues * Removing tests so we can debug them separately * Reinstate test for color only. Gradient needs some async magic
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.
What?
This PR adds a changelog for alterations merged in #45681
It also add a test case for PaletteEdit to ensure that the correct colorpicker is displayed. Picker functionality is covered in existing tests, e.g., packages/components/src/color-palette/test/index.tsx
Update: moving JS unit tests for gradient to a follow up since there is a
"not wrapped in act(...)" warningthat results in a fail. Probably related to the Popover. Tried many available async approaches. Maybe rejigging the component, mocking Popover might work.