OUTLINE-167: outline-core-heading: Heading Component
#403
Conversation
|
❌ Deploy Preview for outlinejs failed.
|
There was a problem hiding this comment.
Remove this entire file. Generated via yarn turbo:version at release time.
There was a problem hiding this comment.
Try running yarn analyze to regenerate README files as needed. If others change, only include updates to this particular component.
| @@ -0,0 +1,40 @@ | |||
| { | |||
| "name": "@phase2/outline-core-heading", | |||
| "version": "0.1.2", | |||
There was a problem hiding this comment.
Actually, last time with core link, it started at 0.0.1., and when the initial release was triggered, it created 0.0.2.
Let's try removing the version line entirely for a NEW package.
himerus
changed the title
outline-core-heading: Heading Component
himerus
added
Why: Reduce adoption friction
Severity: High
Why: Increase standardization
NEW PACKAGE
…T-outline-core-heading
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
himerus
changed the title
outline-core-heading: Heading Component outline-core-heading: Heading Component
| }): TemplateResult { | ||
| return html` | ||
|
|
||
| <slot class=${classMap(classes)}></slot> |
There was a problem hiding this comment.
This will not work. Are there tests/samples in Storybook that show this usage with the classes applied properly?
You cannot directly attach classes to the element. The element is a placeholder inside a web component that users can fill with their own markup. The classes you apply to the element itself do not have any effect on the slotted content.
The classes applied to the element are considered part of the Shadow DOM, not the Light DOM. The Light DOM refers to the normal DOM tree, outside of the Shadow DOM, where the user's content lives. The Shadow DOM is a separate DOM tree attached to an element, providing encapsulation.
…T-outline-core-heading
…ase2/outline into issue/NT-outline-core-heading
WalkthroughThe overall change introduces a new 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: 10
Configuration used: CodeRabbit UI
Files ignored due to filter (5)
- packages/components/outline-core-heading/package.json
- packages/components/outline-core-heading/tsconfig.build.json
- packages/outline-templates/default/src/components/sample/outline-extended-button/package.json
- packages/outline-templates/default/src/components/sample/outline-extended-button/tsconfig.build.json
- yarn.lock
Files selected for processing (12)
- packages/components/outline-core-heading/README.md (1 hunks)
- packages/components/outline-core-heading/index.ts (1 hunks)
- packages/components/outline-core-heading/src/config.ts (1 hunks)
- packages/components/outline-core-heading/src/outline-core-heading.css (1 hunks)
- packages/components/outline-core-heading/src/outline-core-heading.lightdom.css (1 hunks)
- packages/components/outline-core-heading/src/outline-core-heading.ts (1 hunks)
- packages/components/outline-core-heading/src/test/outline-core-heading.test.ts (1 hunks)
- packages/documentation/outline-storybook/config/storybook.main.css (3 hunks)
- packages/documentation/outline-storybook/stories/components/outline-heading.stories.ts (3 hunks)
- packages/outline-templates/default/src/components/sample/outline-extended-button/index.ts (1 hunks)
- packages/outline-templates/default/src/components/sample/outline-extended-button/outline-extended-button.css (1 hunks)
- packages/outline-templates/default/src/components/sample/outline-extended-button/outline-extended-button.ts (1 hunks)
Files skipped from review due to trivial changes (6)
- packages/components/outline-core-heading/index.ts
- packages/components/outline-core-heading/src/outline-core-heading.css
- packages/components/outline-core-heading/src/outline-core-heading.lightdom.css
- packages/documentation/outline-storybook/config/storybook.main.css
- packages/outline-templates/default/src/components/sample/outline-extended-button/index.ts
- packages/outline-templates/default/src/components/sample/outline-extended-button/outline-extended-button.css
Additional comments: 16
packages/components/outline-core-heading/README.md (2)
8-8: The
levelproperty type should be more specific to reflect the allowed heading levels as per the summary which mentionsAllowedHeadingLevels. Please verify if the type should beAllowedHeadingLevelsinstead ofstring | undefined.16-17: The methods
fullMarkupInSlotandgenerateHeadinghave a return type ofTemplateResult<ResultType>. It would be helpful to specify whatResultTyperefers to, or if it is a placeholder, consider providing the actual return type for clarity.packages/components/outline-core-heading/src/config.ts (2)
1-1: Verify the intention behind including
undefinedin theHeadingLevelsarray to ensure it aligns with the component's design requirements.5-27: The configuration for
HeadingSizesandHeadingWeightsis well-defined, ensuring type safety and a clear set of options for these properties.packages/components/outline-core-heading/src/outline-core-heading.ts (1)
- 137-141: The conditional rendering logic in the
rendermethod is correct and follows best practices.packages/components/outline-core-heading/src/test/outline-core-heading.test.ts (3)
4-4: The test suite description uses
outline-headinginstead ofoutline-core-heading. Ensure that the test descriptions accurately reflect the component being tested.10-20: The test 'renders with default values' correctly checks the shadow DOM structure. Ensure that the default rendering of the component matches this structure.
22-25: The test 'renders with slotted content' correctly checks the light DOM for the presence of the slotted text.
packages/documentation/outline-storybook/stories/components/outline-heading.stories.ts (6)
6-14: The import statements and component usage have been correctly updated to reflect the new
outline-core-headingcomponent.15-15: The
levelOptionsarray is correctly defined for use in the Storybook configuration.24-52: The Storybook configuration has been updated to reflect the new properties of the
outline-core-headingcomponent.62-67: The documentation provided in the Storybook parameters offers clear instructions on the usage of the
outline-core-headingcomponent.72-80: The template function and story examples have been updated to use the
outline-core-headingcomponent with the correct properties.93-154: The examples for slotted headings with and without attributes, as well as the Shadow DOM example, have been correctly updated to use the
outline-core-headingcomponent and its properties.packages/outline-templates/default/src/components/sample/outline-extended-button/outline-extended-button.ts (2)
4-5: The commented code and associated comment on lines 4-5 may indicate a version breaking change. It's important to document such changes clearly for future maintainability.
13-14: The
OutlineExtendedButtonclass extendsOutlineCoreButtonbut does not add any new functionality or override any existing methods. If this is intentional and future extensions are planned, it's fine; otherwise, consider adding functionality or documenting the purpose of this extension.
| const el = document.createElement('outline-heading'); | ||
| assert.instanceOf(el, OutlineCoreHeading); |
There was a problem hiding this comment.
The element creation uses outline-heading instead of outline-core-heading. The test should create an instance of the component that matches the class being tested.
- const el = document.createElement('outline-heading');
+ const el = document.createElement('outline-core-heading');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.
| const el = document.createElement('outline-heading'); | |
| assert.instanceOf(el, OutlineCoreHeading); | |
| const el = document.createElement('outline-core-heading'); | |
| assert.instanceOf(el, OutlineCoreHeading); |
| @@ -0,0 +1,14 @@ | |||
| import { OutlineCoreButton } from '@phase2/outline-core-button'; | |||
| import { customElement } from 'lit/decorators.js'; | |||
| import '@phase2/outline-core-button'; | |||
There was a problem hiding this comment.
The import statement for @phase2/outline-core-button on line 3 is redundant since OutlineCoreButton is already imported on line 1 and should be removed to clean up the code.
- import '@phase2/outline-core-button';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.
| import '@phase2/outline-core-button'; |
| // import { buttonVariantsTypes } from "@phase2/outline-core-button/src/outline-core-button"; | ||
| import { buttonVariantsTypes } from '../outline-core-button/src/outline-core-button'; | ||
|
|
||
| buttonVariantsTypes.push('shiny'); |
There was a problem hiding this comment.
Directly modifying an imported array like buttonVariantsTypes on line 8 can have unintended side effects if this array is shared across different modules. Consider using a more controlled approach to extend this array.
| // Rewriting does not work | ||
| // const buttonVariantsTypes = ['shiny', 'dull', 'matte']; |
There was a problem hiding this comment.
The commented out code on lines 10-11 suggests an alternative approach that was not taken. If this code is not needed, it should be removed to avoid confusion.
- // Rewriting does not work
- // const buttonVariantsTypes = ['shiny', 'dull', 'matte'];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.
| // Rewriting does not work | |
| // const buttonVariantsTypes = ['shiny', 'dull', 'matte']; |
| @customElement('outline-core-heading') | ||
| export class OutlineCoreHeading extends OutlineElement { | ||
| static styles = [componentStyles]; | ||
| AdoptedStyleSheets: AdoptedStyleSheets; |
There was a problem hiding this comment.
The AdoptedStyleSheets property should be private and follow camelCase naming conventions.
- AdoptedStyleSheets: AdoptedStyleSheets;
+ private adoptedStyleSheets: AdoptedStyleSheets;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.
| AdoptedStyleSheets: AdoptedStyleSheets; | |
| private adoptedStyleSheets: AdoptedStyleSheets; |
|
|
||
| connectedCallback() { | ||
| super.connectedCallback(); | ||
| this.AdoptedStyleSheets = new AdoptedStyleSheets(globalStyles); |
There was a problem hiding this comment.
After renaming AdoptedStyleSheets to adoptedStyleSheets, update the reference in connectedCallback.
- this.AdoptedStyleSheets = new AdoptedStyleSheets(globalStyles);
+ this.adoptedStyleSheets = new AdoptedStyleSheets(globalStyles);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.
| this.AdoptedStyleSheets = new AdoptedStyleSheets(globalStyles); | |
| this.adoptedStyleSheets = new AdoptedStyleSheets(globalStyles); |
| converter: size => { | ||
| if (size && !Object.values(allowedHeadingSizes).includes(size)) { | ||
| size = allowedHeadingSizes[0]; | ||
| } | ||
| return size; |
There was a problem hiding this comment.
The size converter uses Object.values which is not necessary since allowedHeadingSizes is an array.
- if (size && !Object.values(allowedHeadingSizes).includes(size)) {
+ if (size && !allowedHeadingSizes.includes(size)) {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.
| converter: size => { | |
| if (size && !Object.values(allowedHeadingSizes).includes(size)) { | |
| size = allowedHeadingSizes[0]; | |
| } | |
| return size; | |
| converter: size => { | |
| if (size && !allowedHeadingSizes.includes(size)) { | |
| size = allowedHeadingSizes[0]; | |
| } | |
| return size; |
| converter: weight => { | ||
| if (weight && !Object.values(allowedHeadingWeights).includes(weight)) { | ||
| weight = allowedHeadingWeights[0]; | ||
| } | ||
| return weight; |
There was a problem hiding this comment.
The weight converter uses Object.values which is not necessary since allowedHeadingWeights is an array.
- if (weight && !Object.values(allowedHeadingWeights).includes(weight)) {
+ if (weight && !allowedHeadingWeights.includes(weight)) {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.
| converter: weight => { | |
| if (weight && !Object.values(allowedHeadingWeights).includes(weight)) { | |
| weight = allowedHeadingWeights[0]; | |
| } | |
| return weight; | |
| converter: weight => { | |
| if (weight && !allowedHeadingWeights.includes(weight)) { | |
| weight = allowedHeadingWeights[0]; | |
| } | |
| return weight; |
| if (this.size) { | ||
| slottedHeading?.classList.add(this.size as string); |
There was a problem hiding this comment.
Ensure that this.size is a valid key before adding it as a class.
- slottedHeading?.classList.add(this.size as string);
+ if (allowedHeadingSizes.includes(this.size)) {
+ slottedHeading?.classList.add(this.size);
+ }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.
| if (this.size) { | |
| slottedHeading?.classList.add(this.size as string); | |
| if (this.size) { | |
| if (allowedHeadingSizes.includes(this.size)) { | |
| slottedHeading?.classList.add(this.size); | |
| } |
| if (this.weight) { | ||
| slottedHeading?.classList.add(this.weight as string); |
There was a problem hiding this comment.
Ensure that this.weight is a valid key before adding it as a class.
- slottedHeading?.classList.add(this.weight as string);
+ if (allowedHeadingWeights.includes(this.weight)) {
+ slottedHeading?.classList.add(this.weight);
+ }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.
| if (this.weight) { | |
| slottedHeading?.classList.add(this.weight as string); | |
| if (this.weight) { | |
| if (allowedHeadingWeights.includes(this.weight)) { | |
| slottedHeading?.classList.add(this.weight); | |
| } |
|
TODO: |
Description
<outline-core-heading>Summary by CodeRabbit
New Features
outline-core-headingcomponent with customizable classes, heading levels, sizes, and weights.clickable-cardclass.outline-headingcomponent to ensure proper functionality.Enhancements
outline-storybookwith new CSS variables for improved design customization.outline-headingstories to reflect new component properties and import paths.Documentation
outline-core-headingwith component summary and properties.Style
outline-core-heading.shinyclass inoutline-extended-button.Refactor
OutlineCoreButtonto createOutlineExtendedButtonwith additional variantshiny.