Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: adding more notes #390

Draft
wants to merge 10 commits into
base: next
Choose a base branch
from
1 change: 0 additions & 1 deletion docs/README.md

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -21,27 +21,34 @@ import '@phase2/outline-container';
bottom-margin="spacing-8"
>
## Description
Lit components receive input and store their state as JavaScript class fields or properties.
Outline components receive input and store their state as JavaScript class fields(normally called attributes) or properties.

[Reactive properties](https://lit.dev/docs/components/properties/) are properties that can trigger the reactive update cycle when changed, re-rendering the component, and optionally be read or written to attributes.

## Documentation Status
<outline-alert status="warning" size="large">
<span slot="header">Status: Needs Work/In Progress</span>
<outline-alert status="success" size="large">
<span slot="header">Status: Complete / Update to data May 2023</span>
himerus marked this conversation as resolved.
Show resolved Hide resolved
<p>
This documentation is in need of additional work, or is currently in progress.
</p>
</outline-alert>
</outline-container>

### Property usage basics

- Properties should be commented using [JSDoc](https://github.com/jsdoc/jsdoc).
- Each property should provide a comment including a description of the attribute functionality or purpose.
- Pass to `type` the appropriate [converter](https://lit.dev/docs/components/properties/#conversion-type) (`String`, `Boolean`, etc.).
- Pass to `attribute` an attribute name in kebab-case.
- Attribute names must not collide with HTML standard ones `title`, `href`, etc.
- This means it is best to name properties more like `link-title`, `link-href`, etc.
- Lit [documentation]()
glitchgirl marked this conversation as resolved.
Show resolved Hide resolved

### Slots versus properties
- When do I use a property?
- Properties are used for non-content features that impact the state of the component. For example, active versus inactive states.
glitchgirl marked this conversation as resolved.
Show resolved Hide resolved
- The component shouldn't change its own public properties, except in response to user input. For example, you could set a property to change background color, but the component itself shouldn't change it's own color.
Copy link

@mkochendorfer mkochendorfer May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that I agree with this statement entirely. I agree that the most common case for a component to update a reflected attribute would be based on some user action, but I think there are definitely cases where a component is doing something internally that warrants reflecting its internal state back in the HTML markup regardless of any user action.

Using the background color as an example, what if your component wants to go look up the browser configuration for dark mode and modify the background color that it uses in some way? Or override the default color with a user preference setting it pulls from an API endpoint?

A more realistic example would be timed carousel with a property the reflects the currently active slide index.

I feel like a better callout might be to only use reflect: true on a property when the component will actually modify that property internally, which is a pattern that many components in the repo are already failing to follow correctly, and this is a mistake I see made constantly by extension on my project:
https://github.com/search?q=repo%3Aphase2%2Foutline+%22reflect%3A+true%22&type=code

https://lit.dev/docs/components/properties/#reflected-attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added this discussion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this. A lot of the original components, IIRC had reflect: true on by default as "test mode", and it was argued that it was valuable to have that.

I agree it shouldn't be the default, but used when necessary and appropriate.

- Slots are used for content, such as HTML markup, text, icons, or images.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the above, I think this statement goes to far and also missed in my mind the actual primary use case for slots: flexible regions of a component

Icons in particular it really depends on whether the icons are built into the Outline package itself and thus are set using the name of the icon as a property on a component or if it is an svg upload that the content team does using any svg they wish.

Text often needs to be used as alt text or other things requiring attributes inside of the component, and thus cannot always use slots either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps clarification can be given by some simple scenarios like:

For Example:

  • "if you were building Web Components to be consumed by a React App that does X, Y, and Z, you might do it this way.
  • "if you were building Web Components to be consumed by a Drupal App that does X, Y, and Z, you might do it this way.

I feel like that's probably where some of the assumptions are coming from. Our typical work still being Drupal, we need to account in our docs (and sample integrations would be nice) how to do things with different consumers.

- There are cases in which adding content as a property will make sense, for example using a title to control the state of the component. Use this cautiously, as it can cause issues with a11y tools and automation.
Copy link

@mkochendorfer mkochendorfer May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have specific examples of how this can go wrong with a11y tools? My understanding has been that that was really just a lag in the a11y tools support for web components/shadow dom, which is quickly being addressed as web component usage is exploding across the web with increased browser support.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use this cautiously, as it can cause issues with a11y tools and automation.

I think this relates to how some tools still are unable to pierce the Shadow DOM for inspecting things and may fail a11y tests if we, say build the h1 tag inside the ShadowDOM, but the tool doesn't read it as such.

@glitchgirl correct me if I'm wrong there in your thinking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is exactly correct


### Property with default value

Expand Down