Add missing visually-hidden to work with sample Drupal markup in stories

mike-potter commented 2 years ago

Description

Project stories often use example markup from Drupal (menus, forms, etc). Drupal commonly uses the class visually-hidden to hide elements. This fix adds the class to the outline-element.a11y.css. This mimics what several projects have had to add themselves and should be part of the inherited outline core.

Type of change

Please delete options that are not relevant.

[X] Bug fix (non-breaking change which fixes an issue)
[X] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] This change requires a documentation update

How Has This Been Tested?

Tested on the Baseline Demo site and also being used by IDB.

[X] Visual Testing
[ ] Automated Testing
[ ] Accessibility Testing

Checklist

[x] My code follows the style guidelines of this project
[x] 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
[x] My changes generate no new warnings
[ ] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes
[ ] Any dependent changes have been merged and published in downstream modules

changeset-bot[bot] commented 1 year ago

⚠️ No Changeset found

Latest commit: 6125f9fbe8048ae2a11c3c86f7365b8fd031570f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

tekante commented 1 year ago

I'm not sure I understand the intersection of adding the various .visually-hidden selectors here versus what I think is automatically being added by scripts/styles.js. I think the result is mainly to add ::slotted(.visually-hidden). Should we be adjusting it there instead (or adjusting what that file does based on adding it here)?

Also, it isn't obvious to me that outline-element.a11y.css is used as I don't see it referenced in the styles in packages/outline-core/src/outline-element/outline-element.ts.

scripts/styles.js is what takes the various css files and turns them into the css.lit.ts versions that the component files reference and it's adding some css in by default in createComponentLiterals which applies since this file isn't in a path with css-variables in it . Would it be better to have the code it is adding in this file and expect every component extending to first add in OulineElement.styles and get rid of a bit of css that is showing up by "magic"? That's probably a larger refactor and outside the scope of this change but wanted to get it out there for discussion.

mike-potter commented 1 year ago

Maybe this isn't needed and is from older Outline. In the demo site that used the "legacy" outline, components like outline_dropdown had lines like this:

import a11yStyles from '../outline-element/outline-element.a11y.css.lit';

but looks like in the newer outline this visually_hidden has been added directly to the outline-dropdown.css file. Not sure why it got added directly instead of relying on the stuff added by styles.js. Feels like there is still something to be cleaned up here, but this PR might not be the way to do it.

If styles.js is adding this now, then the visually_hidden added directly in the css file might be redundant.

Still, using this on the Demo baseline until we can migrate to the new Outline and might be an area of code debt that could break stuff to be aware of.

himerus commented 1 year ago

@mike-potter @shaal where are we at on this one? We've updated outline-element, what is left here to verify what we need as defaults?

mike-potter commented 1 year ago

I feel like this can be closed and any issue in the new mono-repo can be opened. I no longer remember why it was necessary to add this class to outline-element and think it was just something that was being done in various project design systems. Seems more like a global styling issue rather than something specific to outline-element

In general I'd be a big fan of removing this kind of "magic" added to every component when it's wrapped by styles.mjs also.

shaal commented 1 year ago

Since any component that extends outlineElement doesn't automatically receive outlineElement's CSS files, we should probably close this. I wish we created our own ~~Tailwind~~ useful utility classes a JIT processor that add these classes only for components who use them.

glitchgirl commented 8 months ago

File removed This CSS is replicated elsewhere

phase2 / outline