Adds Divider web component #26901
Adds Divider web component #26901
Conversation
halerankin
requested review from
a team,
GeoffCoxMSFT,
sopranopillow,
khmakoto,
tomi-msft,
spmonahan,
micahgodbolt,
smhigley,
behowell,
dzearing,
Jacqueline-ms,
Hirse,
JustSlone,
jurokapsiar and
tudorpopams
as code owners
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 355a672:
|
📊 Bundle size report🤖 This report was generated against e793ad8a7f7ae0239a808439d121cf012dd414e5 |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: e793ad8a7f7ae0239a808439d121cf012dd414e5 (build) |
miroslavstastny
requested review from
a team
and removed request for
spmonahan and
tudorpopams
| * Determines the alignment of the content within the divider. Select from center, start, or end. | ||
| */ | ||
| @attr({ attribute: 'align-content' }) | ||
| public alignContent?: DividerAlignContent; |
There was a problem hiding this comment.
@chrisdholt - there is no unified approach to declaring optional attributes. We mix all the following:
badge.appearanceassigns a default value:
progress-bar.shapeis marked as mandatory even though it should not be required:
avatar.avatar-sizein another PR usesType | undefined:
public size: AvatarSize | undefined;- this PR uses optionals.
Can we agree on a single thing? FUIR9 uses optionals.
There was a problem hiding this comment.
Is there a good reason to NOT set a default value?
public appearance: BadgeAppearance = BadgeAppearance.filled;
When I review a spec, one thing I look for is the default state of a given component. When we build and style the component accordingly, the consumer gets the default look and behavior without any further customizations. I would think this is a good thing.
As for optionals, this is a good practice that I picked up while working with another team. If I don't have a default value, I would opt for this approach.
There was a problem hiding this comment.
Is there a good reason to NOT set a default value?
public appearance: BadgeAppearance = BadgeAppearance.filled;
The reason is the reflect mode of Fast @attr.
It is a bidirectional binding, which will reflect any value assignment in Javascript to DOM.
<!-- this is what I author in HTML -->
<fluent-badge></fluent-badge>
<!-- ⬇️ this is what I get in DOM -->
<fluent-badge appearance="filled" color="brand"></fluent-badge>I see 2 problems with this:
- It makes DOM bigger, which might be a performance concern.
- It makes DOM different to HTML, which makes debugging more difficult and confusing ("Where does the
color="brand"come from? I've never set it anywhere."
When I experienced this for the first time, I proposed to use fromView mode for all the attributes but that brings another problem: we depend on the attribute to be present in DOM in our CSS selectors:
So we might use default value in attributes in fromView mode but we still could not depend on them in styles which would make the whole thing even more confusing.
For that reason, me and @chrisdholt decided to not use default attribute values, document what the default is and author the CSS appropriately.
There was a problem hiding this comment.
@miroslavstastny @chrisdholt I just pushed changes for this. Please let me know if this is what you meant by not using default attribute values.
There was a problem hiding this comment.
LGTM! Attributes marked as optional, no default value assigned, but documented in the jsdoc.
Default styles (when the prop is not set) correctly 'render' the default appearance, they never test for the default attribute value (in this case there is no [align-content: 'center'] selector.
👍
|
Thanks for yesterday's feedback @miroslavstastny and @chrisdholt |
change/@fluentui-web-components-b786537a-e6c4-4798-89b5-b0f60dca78e8.json
Outdated
Show resolved
Hide resolved
| * Determines the alignment of the content within the divider. Select from center, start, or end. | ||
| */ | ||
| @attr({ attribute: 'align-content' }) | ||
| public alignContent?: DividerAlignContent; |
There was a problem hiding this comment.
LGTM! Attributes marked as optional, no default value assigned, but documented in the jsdoc.
Default styles (when the prop is not set) correctly 'render' the default appearance, they never test for the default attribute value (in this case there is no [align-content: 'center'] selector.
👍
* Adds Divider component * Generates change
* Adds Divider component * Generates change
* Adds Divider component * Generates change
* Adds Divider component * Generates change
* Adds Divider component * Generates change
* Adds Divider component * Generates change
* Adds Divider component * Generates change
* Adds Divider component * Generates change
* Adds Divider component * Generates change
* Adds Divider component * Generates change
* Adds Divider component * Generates change
* Adds Divider component * Generates change
* Adds Divider component * Generates change
Adds Divider web component.
component spec
Horizontal (default):
Horizontal with content in default position:
Horizontal flex-start:
Horizontal flex-end:
Horizontal inset:
Horizontal, brand color, with icon
Vertical:
Min-height is 20px. Storybook container is much larger than the component itself.
Vertical w content in default position:
Vertical w content, inset:
Vertical flex-start:
Vertical flex-end:
Colors
Strong
Brand
Subtle
