Does not show borders in addon block inputs #5898

wesleybl · 2024-03-19T15:29:26Z

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

netlify · 2024-03-19T15:29:43Z

✅ Deploy Preview for volto canceled.

Name	Link
🔨 Latest commit	`6ed6e6f`
🔍 Latest deploy log	https://app.netlify.com/sites/volto/deploys/65fdec7a9874be0008d1dad0

netlify · 2024-03-19T15:29:44Z

✅ Deploy Preview for plone-components canceled.

Name	Link
🔨 Latest commit	`6ed6e6f`
🔍 Latest deploy log	https://app.netlify.com/sites/plone-components/deploys/65fdec7ad019b80008086e7d

ichim-david · 2024-03-20T06:50:34Z

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

davisagli · 2024-03-20T14:57:21Z

@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 · 2024-03-20T15:06:46Z

@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 · 2024-03-20T15:15:52Z

@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 · 2024-03-20T16:08:26Z

@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 · 2024-03-20T20:51:04Z

@davisagli @ichim-david done.

davisagli

My other doubt here: maybe there are blocks that expect the default focus outline to remain visible?

tiberiuichim · 2024-03-21T07:04:35Z

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

davisagli · 2024-03-21T07:16:40Z

@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 · 2024-03-21T07:23:05Z

@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.

focus-visible-removal.mp4

ichim-david

@wesleybl see my comment #5898 (comment)

sneridagh · 2024-03-21T07:31:54Z

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 · 2024-03-21T14:57:59Z

@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 · 2024-03-21T18:22:19Z

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 · 2024-03-22T06:55:03Z

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 · 2024-03-22T15:38:07Z

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 · 2024-03-22T15:42:23Z

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 · 2024-03-22T19:03:04Z

@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 · 2024-03-22T19:16:56Z

@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 · 2024-03-22T20:41:39Z

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.

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

ichim-david · 2024-03-26T08:10:13Z

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 · 2024-03-26T12:22:23Z

@ichim-david thanks for your help!

sneridagh

LGTM, but asking the @davisagli for a last review.

davisagli

OK with me.

* main: (28 commits) Bump vite from 5.1.5 to 5.1.7 (#5946) Fix pt_BR translation of invalid email message (#5953) Fix markup shortcuts for bold, italic and strikethough fix (#5606) Release 18.0.0-alpha.27 Release @plone/types 1.0.0-alpha.10 Improve color widget picker and types (#5948) Enhanced navigation reducer in Volto (#5817) Release 18.0.0-alpha.26 Rename news item Release @plone/slate 18.0.0-alpha.11 Release @plone/registry 1.5.5 Release @plone/types 1.0.0-alpha.9 docs: Cleanup obsolete EEA projects and update info about EEA main website (#5943) Bump vite from 5.1.4 to 5.1.5 (#5942) Add a new label `needs: triage` to new bug reports (#5940) Fix redirect of `https://sustainability.eionet.europa.eu` to `https:/… (#5941) Does not show borders in addon block inputs (#5898) Fix edge case in search options mangling when the options are false-ish (#5869) Add additional parameters to ContentsUploadModal to be reusable in different scenarios (#5881) fix(slate): fix insert/remove element edgecase bug in slate (#5926) ...

wesleybl requested review from tiberiuichim and sneridagh March 19, 2024 15:29

wesleybl requested a review from iFlameing March 19, 2024 15:30

wesleybl force-pushed the no_border branch from 0624cd0 to 22ab883 Compare March 19, 2024 15:34

tiberiuichim approved these changes Mar 20, 2024

View reviewed changes

wesleybl requested review from davisagli and ichim-david March 20, 2024 20:51

davisagli reviewed Mar 21, 2024

View reviewed changes

ichim-david requested changes Mar 21, 2024

View reviewed changes

wesleybl force-pushed the no_border branch from 768f11f to f8575d6 Compare March 22, 2024 18:59

wesleybl requested review from davisagli and ichim-david March 22, 2024 19:03

wesleybl force-pushed the no_border branch from f8575d6 to 6ed6e6f Compare March 22, 2024 20:39

Does not show borders in addon block inputs

6ed6e6f

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

ichim-david approved these changes Mar 26, 2024

View reviewed changes

ichim-david added the 24 status: ready label Mar 29, 2024

sneridagh approved these changes Mar 31, 2024

View reviewed changes

davisagli approved these changes Apr 1, 2024

View reviewed changes

davisagli merged commit 13d5d60 into main Apr 1, 2024
74 checks passed

davisagli deleted the no_border branch April 1, 2024 05:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Does not show borders in addon block inputs #5898

Does not show borders in addon block inputs #5898

wesleybl commented Mar 19, 2024

netlify bot commented Mar 19, 2024 •

edited

Loading

netlify bot commented Mar 19, 2024 •

edited

Loading

ichim-david commented Mar 20, 2024

davisagli commented Mar 20, 2024

wesleybl commented Mar 20, 2024 •

edited

Loading

ichim-david commented Mar 20, 2024 •

edited

Loading

wesleybl commented Mar 20, 2024

wesleybl commented Mar 20, 2024

davisagli left a comment

tiberiuichim commented Mar 21, 2024

davisagli commented Mar 21, 2024

ichim-david commented Mar 21, 2024

ichim-david left a comment

sneridagh commented Mar 21, 2024

wesleybl commented Mar 21, 2024

wesleybl commented Mar 21, 2024

ichim-david commented Mar 22, 2024

wesleybl commented Mar 22, 2024

wesleybl commented Mar 22, 2024

wesleybl commented Mar 22, 2024

ichim-david commented Mar 22, 2024 •

edited

Loading

wesleybl commented Mar 22, 2024

ichim-david commented Mar 26, 2024

wesleybl commented Mar 26, 2024

sneridagh left a comment

davisagli left a comment

Does not show borders in addon block inputs #5898

Does not show borders in addon block inputs #5898

Conversation

wesleybl commented Mar 19, 2024

netlify bot commented Mar 19, 2024 • edited Loading

✅ Deploy Preview for volto canceled.

netlify bot commented Mar 19, 2024 • edited Loading

✅ Deploy Preview for plone-components canceled.

ichim-david commented Mar 20, 2024

davisagli commented Mar 20, 2024

wesleybl commented Mar 20, 2024 • edited Loading

ichim-david commented Mar 20, 2024 • edited Loading

wesleybl commented Mar 20, 2024

wesleybl commented Mar 20, 2024

davisagli left a comment

Choose a reason for hiding this comment

tiberiuichim commented Mar 21, 2024

davisagli commented Mar 21, 2024

ichim-david commented Mar 21, 2024

ichim-david left a comment

Choose a reason for hiding this comment

sneridagh commented Mar 21, 2024

wesleybl commented Mar 21, 2024

wesleybl commented Mar 21, 2024

ichim-david commented Mar 22, 2024

wesleybl commented Mar 22, 2024

wesleybl commented Mar 22, 2024

wesleybl commented Mar 22, 2024

ichim-david commented Mar 22, 2024 • edited Loading

wesleybl commented Mar 22, 2024

ichim-david commented Mar 26, 2024

wesleybl commented Mar 26, 2024

sneridagh left a comment

Choose a reason for hiding this comment

davisagli left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 19, 2024 •

edited

Loading

netlify bot commented Mar 19, 2024 •

edited

Loading

wesleybl commented Mar 20, 2024 •

edited

Loading

ichim-david commented Mar 20, 2024 •

edited

Loading

ichim-david commented Mar 22, 2024 •

edited

Loading