search

plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
426 stars 576 forks source link

Does not show borders in addon block inputs #5898

Closed wesleybl closed 3 weeks ago

wesleybl commented 1 month ago

We made the css selector more generic, so that it can also be applied to addon blocks.

Fixes #5894

netlify[bot] commented 1 month ago

Deploy Preview for volto canceled.

Name Link
Latest commit 6ed6e6f574eac2535e05bf54f2dae682c7cc4b4a
Latest deploy log https://app.netlify.com/sites/volto/deploys/65fdec7a9874be0008d1dad0
netlify[bot] commented 1 month ago

Deploy Preview for plone-components canceled.

Name Link
Latest commit 6ed6e6f574eac2535e05bf54f2dae682c7cc4b4a
Latest deploy log https://app.netlify.com/sites/plone-components/deploys/65fdec7ad019b80008086e7d
ichim-david commented 1 month ago

@wesleybl you wrote a cypress test to show some css changes, man that is dedication :) I salute you.

davisagli commented 1 month ago

@wesleybl you wrote a cypress test to show some css changes, man that is dedication :) I salute you.

I appreciate the effort but I would be careful about doing a lot of this. Each new cypress test adds to the amount of time it takes to run the acceptance tests on every pull request. IMO It would make more sense to add the assertion at the end of an existing test that already creates a table block, instead of adding a new test that does the steps to create the block again.

wesleybl commented 1 month ago

@davisagli testing times are already high. Having a few more seconds won't make much difference to the total time. We often let the PR tests run and go have a drink of water. I think this is better than "contaminating" existing tests with something that is not related to that test.

I only added these tests because it was a side effect of a lib update. So I did a test to make sure future updates don't cause any problems.

ichim-david commented 1 month ago

@davisagli testing times are already high. Having a few more seconds won't make much difference to the total time. We often let the PR tests run and go have a drink of water. I think this is better than "contaminating" existing tests with something that is not related to that test.

@wesleybl I know I praised your effort and although I got @davisagli again somehow to disagree with my judgment :), I actually tend to think he is right regarding checking if there is an existing test and adding another assert statement. If we look at cypress best practices documentation we have https://docs.cypress.io/guides/references/best-practices#Creating-Tiny-Tests-With-A-Single-Assertion Here you have the following statement "It is common for tests in Cypress to issue 30+ commands. Because nearly every command has an implicit assertion (and can therefore fail), even by limiting your assertions you're not saving yourself anything because any single command could implicitly fail."

wesleybl commented 1 month ago

@davisagli @ichim-david Okay I'll change that. In any case, some blocks did not have tests. So the new test will have to be small in these cases.

wesleybl commented 1 month ago

@davisagli @ichim-david done.

tiberiuichim commented 1 month ago

@davisagli the blocks already have their own border indicator for "active/focused block"

davisagli commented 1 month ago

@tiberiuichim then it makes sense when the slate editor covers the entire area of the block, but that's not always the case.

ichim-david commented 1 month ago

@davisagli the blocks already have their own border indicator for "active/focused block"

@wesleybl @tiberiuichim Actually @davisagli is right, it seems I am becoming his cheerleader again :). Although @wesleybl your intention is good the problem is that [class^="block-editor-"] :focus-visible mean that any children inside the block-editor-* divs should have 0 outline in focus-visible.

I have made a video that showcases the effect of this rule on the focus indicator when trying to tab around. You will see that we lose the outline for the drag and drop icon and inside the image block to know that we are inside it. If we have problems with input and outline maybe we could restrict the removal of the outline only to them instead of having a generic removal since you will be affected more than you realize.

https://github.com/plone/volto/assets/152852/2cfc9e05-ed17-4caa-96cc-0e4ba5f0c703

sneridagh commented 1 month ago

I am +1 with @ichim-david we have to be careful about a11y in these ones. Not an expert here, I can't help much. When in doubt, I always rely on taking a look how the Adobe guys do it in RAC.

wesleybl commented 1 month ago

@davisagli @ichim-david Ok. Can I create a new class "block-editor-no-outline" and add this class to blocks that cannot have an outline? So I can use this class in addos.

wesleybl commented 1 month ago

Can I create a new class "block-editor-no-outline" and add this class to blocks that cannot have an outline?

This could be done with a new classesInEdit block setting. What do you think?

ichim-david commented 1 month ago

Can I create a new class "block-editor-no-outline" and add this class to blocks that cannot have an outline?

This could be done with a new classesInEdit block setting. What do you think?

@wesleybl I am going to defer to the decision of @davisagli, @sneridagh and @tiberiuichim, naming is hard and if we are to introduce new block settings I am not comfortable saying yay or nay.

I am looking better at the issue you've added and the outline problem that was flagged by @tiberiuichim in the slate update https://github.com/plone/volto/pull/5186/files#r1358197824 it would make sense to have the ability to have a generic slate class like tibi asked here and if that is present on that one you remove the outline.

At the same time you run into issues with an add-on input and that can easily be fixed by having a simple css rule in your blocks.less equivalent and I don't know if it's worth trying todo something about it in core.

As we get into comments about pros on cons we might run into situations where we deem that it's better not to introduce some change such as the pull request with changing the toolbar icon from user to the cog.

I hope that we can avoid this scenario, I would think that it would be useful to see how to add a generic class to slate fields but again maybe the other core developers have a better suggestion than what I write here, so let's wait and see what they will say.

wesleybl commented 1 month ago

I would think that it would be useful to see how to add a generic class to slate fields

@ichim-david the Slate doesn't have a class, but I can pass the style property to it:

https://github.com/ianstormtaylor/slate/blob/c2ae1eda91d0aae1cd63bd46af759c542c292a8a/packages/slate-react/src/components/editable.tsx#L118

I think I'll do it like this.

wesleybl commented 1 month ago

the Slate doesn't have a class, but I can pass the style property to it

humm but that wouldn't solve my problem that the all Slate in Volto doesn't have border.

wesleybl commented 1 month ago

@ichim-david @davisagli @tiberiuichim the Slate has no class but has a div with the data-slate-editor attribute. So I used this attribute in the CSS selector to identify the Slate. I also added a test to ensure the border appears in the image block.

Can you take a look please?

ichim-david commented 1 month ago

@ichim-david @davisagli @tiberiuichim the Slate has no class but has a div with the data-slate-editor attribute. So I used this attribute in the CSS selector to identify the Slate. I also added a test to ensure the border appears in the image block.

Can you take a look please?

@wesleybl it's Friday night 21:15PM so no test for me tonight we'll test it tomorrow morning. Using the has selector to make something generic is clever, I don't know if we need to bother anymore about support for has selector in browsers, maybe the existing selectors can be kept for the older browsers who might not have support for has.

wesleybl commented 1 month ago

I don't know if we need to bother anymore about support for has selector in browsers, maybe the existing selectors can be kept for the older browsers who might not have support for has.

@ichim-david I changed the code only to:

[data-slate-editor='true'] {
  outline: none;
}

This works and we don't need to use has.

ichim-david commented 1 month ago

I don't know if we need to bother anymore about support for has selector in browsers, maybe the existing selectors can be kept for the older browsers who might not have support for has.

@ichim-david I changed the code only to:

[data-slate-editor='true'] {
  outline: none;
}

This works and we don't need to use has.

@wesleybl indeed it does, I tested locally with volto-18 and every block available and I had focus-visible present where needed as such this is the cleanest possible fix without crazy selectors and messing with the focus-visible. Accepting the work from my part, to be determined the judgment of the other reviewers.

wesleybl commented 1 month ago

@ichim-david thanks for your help!