-
Notifications
You must be signed in to change notification settings - Fork 81
docs: align 't' components with style guide #13707
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
base: dev
Are you sure you want to change the base?
Conversation
geospatialem
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨📚 ✨
Few follow-ups and one proposed change is dependent on some of our internal discussions.
There may be additional considerations with some of the following:
tab-nav.tsx,tab-title.tsx,table.tsx,table-cell.tsx,text-area.tsx,time-picker.tsx-messageOverridespropertytab-nav.scss- there is an extra space aftericonStart,in--calcite-tab-text-colortime-picker.tsx- thestepandvaluehave content in parenthesis, they may be okay as-is, maybe a closer look to confirm? 👀
| * @prop --calcite-tab-background-color: Specifies the component's background color. | ||
| * @prop --calcite-tab-background-color-hover: When `calcite-tabs` is `bordered`, specifies the component's background color when hovered. | ||
| * @prop --calcite-tab-accent-color-press: Specifies the component's accent color when selected or active. | ||
| * @prop --calcite-tab-accent-color-press: When `selected` or active, specifies the component's accent color. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While testing the component, since we only allow for one selected tab-title, maybe this description should be left as-is for readability purposes:
Specifies the component's accent color when selected or active.
cc @matgalla for 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geospatialem can you help me understand why the number we allow to be selected changes things?
I also think my changes are consistent with guidance Matt mentioned in my PR for S components to use the format "When [prop] or [state], stuff happens..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you help me understand why the number we allow to be selected changes things?
Matt's comment came in after mine did yesterday, so my comment is outdated based on the more recent conversations.
While testing the component, since we only allow for one selected
tab-title, maybe this description should be left as-is for readability purposes:Specifies the component's accent color when selected or active.
cc @matgalla for 👀
The reason for the comment above is the selected property can only be applied to one tab-title. Since the majority of use cases rely on a user activating the tab-title rather than programmatically setting it, I was wondering if we should call out the property name (with backticks), but flexible on either language as long as we have alignment with @matgalla.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you raise a good point @geospatialem! True, users probably won't be setting the selected tab often, but there may also be an expectation of the prop name in backticks since we've established that format elsewhere. If it wasn't the same prop name I'd be in agreement, but I'm leaning towards the wording consistency with stepper at first glance here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More food for thought - something like chip/chip-group might act as a gray area in between both cases here. chip-group can have selection-mode=single|multiple and --calcite-chip-select-icon-color-press is used to specify the color of the selected elements for both. Would we expect users to apply selected props to chips more or less often based on these options?
The current wording for chip's prop description is a bit unique by necessity, but I'm assuming it will be a part of #12833 as well. I'm tempted to say we just always lead with "When selected, " but is that awkward saying "selected" twice in the chip prop description?
When
selected, specifies the component's selection element icon color.
Orrr maybe this is another example of a component where users wouldn't often specify an initial selected chip? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thought for now - do we need "or active" in prop descriptions that are scoped to selected props? Do they both need to be mentioned or could we remove it?
| * @prop --calcite-table-selection-cell-background-color-selected: Specifies the background color of the component's `selectionMode` cell. | ||
| * @prop --calcite-table-selection-cell-icon-color: Specifies the icon color of the component's `selectionMode` selection icon. | ||
| * @prop --calcite-table-selection-cell-icon-color-selected: Specifies the icon color of the component's `selectionMode` selection icon when selected. | ||
| * @prop --calcite-table-selection-cell-icon-color-selected: Specifies the icon color of the component's `selectionMode` selection icon for `selected` `calcite-table-row`s,. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still is a bit hard to read through, will let others post considerations for readability. cc @Esri/calcite-pes
One copyedit with the "," character at the trailing end:
| * @prop --calcite-table-selection-cell-icon-color-selected: Specifies the icon color of the component's `selectionMode` selection icon for `selected` `calcite-table-row`s,. | |
| * @prop --calcite-table-selection-cell-icon-color-selected: Specifies the icon color of the component's `selectionMode` selection icon for `selected` `calcite-table-row`s. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "selectionMode selection icon" part is particularly difficult to read. Maybe we don't need to use the property in this case. The verbiage for selectionMode doesn't mention the selection icons, so I don't think it's a helpful addition.
| * @prop --calcite-table-selection-cell-icon-color-selected: Specifies the icon color of the component's `selectionMode` selection icon for `selected` `calcite-table-row`s,. | |
| * @prop --calcite-table-selection-cell-icon-color-selected: Specifies the icon color of the component's selection icon on `selected` `calcite-table-row`s. |
I prefer the above, but if we wanted to keep selectionMode maybe this would be better.
| * @prop --calcite-table-selection-cell-icon-color-selected: Specifies the icon color of the component's `selectionMode` selection icon for `selected` `calcite-table-row`s,. | |
| * @prop --calcite-table-selection-cell-icon-color-selected: On `selected` `calcite-table-row`s, specifies the icon color of the component's selection icon that is associated with the component's `selectionMode`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the above, but if we wanted to keep
selectionModemaybe this would be better.
+1 for the above example 🚀
- @prop --calcite-table-selection-cell-icon-color-selected: Specifies the icon color of the component's selection icon on
selectedcalcite-table-rows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geospatialem can you clarify if you're wanting the one with selectionMode or without?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geospatialem can you clarify if you're wanting the one with
selectionModeor without?
Updated this comment, am referring to the first instance without selectionMode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I was making this change I noticed that many of the table tokens needed additional clarification on when they were applicable. For this one I prepended "When selectionMode is "multiple" or "single"," because it is not applicable when selectionMode is "none".
When
selectionModeis"multiple"or"single", specifies the icon color of the component's selection icon onselectedcalcite-table-rows.
Related Issue: #12833
Summary
Audited the following components for consistency with the Style Guide:
tab-navtab-titletable-rowtabletabstext-areatile-grouptiletime-pickertooltiptree-item