release/0.0.1 to main

[chrishbite]

Description

This PR serves as the initial 0.0.1 version release of this plugin and should be considered in a prototype stage at this point.

The Image Comparison plugin consist of 2 new registered blocks, bigbite/image-comparison-item and bigbite/image-comparison which work together to allow two images to be visualised side by side via either a horizontal or vertical draggable divider line.

A number of settings are available to change the style of this feature.

Steps to test

No specific steps are available, please test the plugins available functionality in both the editor and frontend.

Screenshots/Videos

http://bigbite.im/v/Dr8qmf

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules

[g-elwell]

I love this idea @chrishbite, it works great too... fantastic job!

Most of the code is perfect, but I think there may be some changes made to CSS naming to align with our standards, and hopefully avoid nested SCSS.

I've made some inline comments but thought I'd also share my overall suggestions and the approach I'd take in relation to this.

I'd recommend the following structure/class naming:

<figure ... class="... wp-block-bigbite-image-comparison wp-block-bigbite-image-comparison--horizontal">
    <div class="wp-block-bigbite-image-comparison__container">
        <img ... class="... wp-block-bigbite-image-comparison-item" />
        <img ... class="... wp-block-bigbite-image-comparison-item" />
        <div class="wp-block-bigbite-image-comparison__divider">
            <button type="button" aria-label="Divider Button">
                <span></span>
                <span></span>
            </button>
        </div>
    </div>
</figure>

Changes here include

• Use of both -- to denote modifiers and __ to denote children
• Consistent use of wp-block prefix so that our modifiers/children match up to the original block class name.
• Shortened bigbite-image-comparison--divider--horizontal-axis to bigbite-image-comparison--horizontal. This is minor, but class names are getting quite long (this is the downside of using BEM) so shortening them where we can without losing meaning is good.

Then we should remove nested selectors in SCSS, since BEM should mean we don't need to introduce extra specificity and our standards recommend avoiding nesting as much as possible, see here for more info.

It might just be me, but parsing & + & sibling within the code takes me out of the flow, but I think this is more to do with & than sibling selectors themselves, so removing nesting should make this more readable at the same time IMO.

Other than that, I do have some concerns over a11y, which I feel might require some research to get right. Having keyboard support and accessible labels for screen reader users are both a must here IMO. I don't have the answers to that problem myself as this is a less common pattern so resources are scarce, but did find this comment from a screen reader user on SO which points to some approaches that may be applicable.

That said, I would be completely happy to approve this PR once the other changes are addressed if you wanted to create a separate issue to research and address a11y requirements.

Similarly, it would also be great to see an issue go in relating to documentation. I don't want to block this PR by asking for it here, but I feel this is such a useful block that it would be nice to have a bit more detail fleshed out and perhaps a visual demo in the readme.

Oh, lastly I would have LOVED to see this use the interactivity API, is that something you've had a chance to look into yet? I'd be interested to see what that would look like and how it would compare with the existing frontend JS approach. Maybe a future exploration idea 👀

Again you've done a fantastic job here, a brilliant idea well executed 🎉

[jaymcp]

Great work on this so far; this is a fantastic start! It looks really good, and works well... when using a mouse. There are some accessibility issues that I think need addressing before I can approve the PR, though. I've left comments inline for this and other issues I've spotted.

[chrishbite]

Hey @g-elwell @jaymcp thanks for the feedback on this, much appreciated!

I've actioned the majority of the feedback I think so this should get us closer towards a finished plugin.

It looks really good, and works well... when using a mouse

@jaymcp Keyboard navigation functionality has been implemented on this now 🎉 ee7f845

Other than that, I do have some concerns over a11y
There are some accessibility issues that I think need addressing before I can approve the PR

@jaymcp @g-elwell Yeah I'm in total agreement with all the points mentioned on accessibility and it does need to be looked at as a priority going forward. As discussed, due to time constraints I just didn't have the capacity to implement as much as I would of liked to for this initial PR, including the accessibility work.

However I have implemented keyboard navigation functionality recently to this PR as a base for the future accessibility work. ee7f845.

Similarly, it would also be great to see an issue go in relating to documentation. I don't want to block this PR by asking for it here, but I feel this is such a useful block that it would be nice to have a bit more detail fleshed out and perhaps a visual demo in the readme.

@g-elwell Yeah definitely, I do have a list of issues that I need to create for work that I considered MVP but was unable to complete due to time, the documentation is amongst those.

Oh, lastly I would have LOVED to see this use the interactivity API, is that something you've had a chance to look into yet? I'd be interested to see what that would look like and how it would compare with the existing frontend JS approach. Maybe a future exploration idea

@g-elwell I did have a quick look but as I've not used that particular API yet I didn't want to slow down on the prototype work for this while I became familiar with it, I do agree this is an avenue worth exploring in the future though if there is benefits to be gained.

I think the next steps on this and if everyone agrees would be to merge this PR (as long as there's no required feedback on any of the changes implemented), create the MVP issues and then involve the wider internal team for contributions as I'm keen to see this reach a stable 1.0.0 release quickly.

[g-elwell]

@chrishbite I agree with your comments, thanks again for the effort you've put into this. I wasn't fully aware of your time constraints previously, so I think we should be distinguishing between what is a blocker for this PR and what is a blocker for the 1.0.0 release.

As it stands, there's nothing in this PR which is preventing it from going forward IMO and I'm happy to chip in creating any required issues so the wider team can pick things up from here 🎉

[jaymcp]

Thanks for addressing so much of the feedback, @chrishbite!
As we've discussed, we're approving this PR as-is, and will be opening issues to address the outstanding tasks.