fix: adding more notes #390
Conversation
|
✅ Deploy Preview for outlinejs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
packages/outline-docs/src/guides/development/component-development/06-properties.mdx
Outdated
Show resolved
Hide resolved
packages/outline-docs/src/guides/development/component-development/06-properties.mdx
Outdated
Show resolved
Hide resolved
…ment/06-properties.mdx Co-authored-by: Ofer Shaal <[email protected]>
packages/outline-docs/src/guides/component-development/06-properties.mdx
Show resolved
Hide resolved
packages/outline-docs/src/guides/component-development/06-properties.mdx
Outdated
Show resolved
Hide resolved
packages/outline-docs/src/guides/component-development/06-properties.mdx
Outdated
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. | ||
| - 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
added this discussion
There was a problem hiding this comment.
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.
| - 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. | ||
| - 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. | ||
| - Slots are used for content, such as HTML markup, text, icons, or images. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| - Properties are used for non-content features that impact the state of the component. For example, active versus inactive states. | ||
| - 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. | ||
| - Slots are used for content, such as HTML markup, text, icons, or images. | ||
| - 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
that is exactly correct
WalkthroughThe recent update introduces comprehensive documentation and guidelines for component development within the Outline framework and Lit2. It covers a wide range of topics from the basic structure and styles of components, to more advanced concepts such as lifecycle methods, properties, slots, and event handling. Additionally, it provides resources on extending components and utility functions, alongside a file dedicated to additional resources and tools for web development. Changes
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (12)
- packages/outline-docs/src/guides/99-additional-resources.mdx (1 hunks)
- packages/outline-docs/src/guides/component-development/01-component-structure.mdx (1 hunks)
- packages/outline-docs/src/guides/component-development/03-styles.mdx (1 hunks)
- packages/outline-docs/src/guides/component-development/04-controllers.mdx (1 hunks)
- packages/outline-docs/src/guides/component-development/05-render.mdx (1 hunks)
- packages/outline-docs/src/guides/component-development/06-properties.mdx (1 hunks)
- packages/outline-docs/src/guides/component-development/07-slots.mdx (1 hunks)
- packages/outline-docs/src/guides/component-development/08-lifecycle-methods.mdx (1 hunks)
- packages/outline-docs/src/guides/component-development/09-events.mdx (1 hunks)
- packages/outline-docs/src/guides/component-development/10-utility-functions.mdx (1 hunks)
- packages/outline-docs/src/guides/component-development/11-extending.mdx (1 hunks)
- packages/outline-docs/src/guides/component-development/main.mdx (1 hunks)
| ```typescript | ||
| private utilityFunction = (data: string[]) => { | ||
| data.forEach(str: string=> console.log(str)) | ||
| } |
There was a problem hiding this comment.
Syntax error in the TypeScript code block.
- data.forEach(str: string=> console.log(str))
+ data.forEach((str: string) => console.log(str)); The arrow function syntax is incorrect. It should have parentheses around the parameter and a semicolon at the end of the statement.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ```typescript | |
| private utilityFunction = (data: string[]) => { | |
| data.forEach(str: string=> console.log(str)) | |
| } | |
| private utilityFunction = (data: string[]) => { | |
| data.forEach((str: string) => console.log(str)); | |
| } |
glitchgirl
changed the base branch from
next
to
feature/outline-core-link-updates
|
Marking this as Draft, as the system is thinking these are all new files, not to be merged with the updates I had made and moving things around, etc. |
himerus
added
CANNOT MERGE
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist
Summary by CodeRabbit