Closed kienstra closed 4 years ago
Hi @mindctrl and @mikemcalister, Good to see you earlier.
Could you please review this when you have a chance? The top comment is up to date with testing instructions.
Thanks!
Oh, sorry. It look like I need to resolve the merge conflicts.
@kienstra sorry, I caused this conflict by merging the other Circle CI config work.
On the IconButton
deprecation, that appears to have happened in WP 5.4. We currently support 5.3 according to the readme file. I'm not opposed to bumping the requirement, especially since the block editor is progressing very quickly and causing divergences like we see here. Just wanted to note that so we could all chat about it.
Hi @mindctrl! Thanks for looking at this!
@kienstra sorry, I caused this conflict by merging the other Circle CI config work.
develop
and resolve the conflict.On the IconButton deprecation, that appears to have happened in WP 5.4. We currently support 5.3 according to the readme file. I'm not opposed to bumping the requirement, especially since the block editor is progressing very quickly and causing divergences like we see here. Just wanted to note that so we could all chat about it.
5.3
if you'd like. Whatever you prefer. The e2e tests pass with <IconButton>
, when I added a .wp-env.json
file:{
"core": "WordPress/WordPress#5.3.2",
"plugins": [ "." ]
}
Sure, it'd be easy to test 5.3 if you'd like.
@kienstra ignore my comment. It looks like Button
works in 5.3. They made it available prior to deprecating IconButton
.
All good, thanks for looking into that.
This works great. Nice work @kienstra!
I noticed a couple visual issues in the block editor in WP 5.3, related to the icon button change (I think, didn't dig into it). One issue exists already in develop
on WP 5.3, but the others appear to be new.
Previous:
In this PR:
Previous:
In this PR:
Already existed in 5.3, but may be quickly addressable here if it's related. We could also open another issue.
Author Profile Box
In WP 5.4
In WP 5.3
Hi @mindctrl, Good catch, thanks for testing that.
I'll look at it soon.
Hi @mindctrl, Thanks for bringing up that issue with the icon not displaying. I also saw it on the blocks you mentioned, like:
It looks like the <Button>
that WP 5.3 has doesn't use have an icon prop like the most recent version does.
So this passes an <Icon>
as a child of the <Button>
, like the new version of Button does.
There are also some styling fixes here for that.
The <IconButton>
used to have padding: 8px
, which helped that 'Select Image' button in the screenshot above not look so small.
So this PR adds some padding: 8px
. Let me know if you'd prefer a different approach.
Thanks, John!
Hi @mindctrl, Good catch.
Is this what you had in mind? Now both WP Core 5.3.2 and 5.4.1 look like this:
If you were thinking the opposite, I could easily change that.
@kienstra looks good!
Nice, thanks!
@mikemcalister, thanks a lot for reviewing this.
Let me know if there are any flaky tests in the future. Sometimes it takes a few tweaks, but overall I think it's helpful to have lightweight e2e tests.
@kienstra Will do! Definitely agree on the lightweight e2e tests.
Thanks, John! It's good to see the purple 😄
wp-env
andwp-scripts
..circleci/config.yml
for thisHow to test
Documentation
PR includes (basic) documentation.
Suggested changelog entry
Notes
Feel free to comment, especially if there's something that would be good to test.