-
Notifications
You must be signed in to change notification settings - Fork 86
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
doc: refactors props for multiple component documentation in storybook #2370
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for marvelous-moxie-a6e2fe ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…om brand header component
readOnly: { type: Boolean }, | ||
transparent: { type: Boolean }, | ||
ariaLabelSelected: { type: String }, | ||
hcmLabelDisabled: { type: String }, |
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.
we can also add the description for these attributes here :
e.g. for hcmLabelDisabled
https://github.com/telekom/scale/blob/main/packages/components/src/components/dropdown-select/readme.md?plain=1#L17
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.
my suggestion is to move the userNavigation
and iconNavigation
that get added here outside the prop table and add them as extra text under the table.
@@ -26,6 +29,9 @@ export default { | |||
helperText: { type: String }, | |||
fullWidth: { type: Boolean, default: false }, | |||
styles: { type: String }, | |||
selectedIndex: { type: Number }, | |||
ariaLabelTranslation: { type: `segment button with $slottedSegments` }, |
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.
also here the prop description could help
| `ariaLabelTranslation` | `aria-label-translation` | (optional) aria-label attribute needed for icon-only buttons | `string` | ``segment button with $slottedSegments`` | |
@@ -2,11 +2,12 @@ | |||
export default { | |||
name: 'scale-sidebar-nav', | |||
props: { | |||
ariaLabel: { type: String }, | |||
ariaLabelSiderbarNav: { type: String }, |
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.
prop description need to be added for all of these props:
| `ariaLabelSidebarNav` | `aria-label-sidebar-nav` | From mdn: A brief description of the purpose of the navigation, omitting the term "navigation", as the screen reader will read both the role and the contents of the label. | `string` | `undefined` | |
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.
Please update the descriptions to include the prop description from readme
| `customColor` | `custom-color` | <span style="color:red">**[DEPRECATED]**</span> (optional) slider custom color<br/><br/> | `string` | `undefined` | |
@@ -39,7 +39,6 @@ | |||
:focusable="focusable" | |||
:decorative="decorative" | |||
:accessibility-title="accessibilityTitle" | |||
:styles="styles" |
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.
add the style back here to make the storybook work
@@ -55,7 +54,6 @@ export default { | |||
focusable: { type: Boolean, default: false }, | |||
decorative: { type: Boolean, default: false }, | |||
accessibilityTitle: { type: String }, | |||
styles: { type: String } |
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.
add the style back here to make the storybook work
@@ -32,11 +32,12 @@ export const Template = (args, { argTypes }) => ({ | |||
template: ` | |||
<div :style="wrapperStyles"> | |||
<scale-sidebar-nav | |||
:arial-label="ariaLabel" | |||
:arial-label-sidebar-nav="ariaLabelSiderbarNav" |
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.
please add the readme description to the props , as they exist
preselected: { | ||
table: { | ||
disable: true, | ||
}, | ||
description: '', | ||
control: { type: null }, | ||
}, |
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.
add this again, as it helps render the component
@@ -12,7 +12,7 @@ import TabPanel from './TabPanel.vue'; | |||
<Meta | |||
title="Components/Tab Navigation" | |||
component={ScaleTabNav} | |||
subcomponents={{ 'Scale Tab Header': TabHeader, 'Scale Tab Panel': TabPanel }} |
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.
why did you remove the tabHeader
section here?
…tion in storybook
@@ -117,7 +117,7 @@ Du kannst ganz einfach die gleichen Ergebnisse erzielen: | |||
```html | |||
<scale-text-field | |||
:value="example" | |||
@scale-change="example = $event.target.value" |
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.
Make the event be kebab case again , as that is the correct way to call the events
disabled: { type: Boolean, default: false }, | ||
preselected: { type: Boolean, default: false }, | ||
withIcon: { type: Boolean, default: true }, | ||
size: { type: String, default: 'small' }, | ||
small: { type: Boolean, default: false }, | ||
small: { type: Boolean, default: false}, | ||
size: { type: 'small' | 'large' , default: 'small' }, |
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.
Revert this to
disabled: { type: Boolean, default: false },
preselected: { type: Boolean, default: false },
withIcon: { type: Boolean, default: true },
size: { type: 'small' | 'large' , default: 'small' },
small: { type: Boolean, default: false },
}} | ||
/> | ||
|
||
export const Template = (args, { argTypes }) => ({ | ||
components: { ScaleTabNav }, |
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.
Revert this. Since it is breaking the tests
table: { | ||
type: { summary: 'string' }, | ||
}, | ||
description: `(optional) size`, |
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.
revert this as well
@@ -10,6 +10,7 @@ | |||
:meta-nav-external-aria-label="metaNavExternalAriaLabel" | |||
:lang-switcher-aria-label="langSwitcherAriaLabel" | |||
:main-nav-aria-label="mainNavAriaLabel" | |||
:type="type" |
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.
remove this line from here to fix the story
@@ -32,7 +33,8 @@ export default { | |||
metaNavAriaLabel: String, | |||
metaNavExternalAriaLabel: String, | |||
langSwitcherAriaLabel: String, | |||
mainNavAriaLabel: String, | |||
mainNavAriaLabel: String, | |||
type: String, |
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.
remove this line from here to fix the story
@@ -432,7 +434,8 @@ Set the profile menu's `logged-in` property to `true` or `false` to switch betwe | |||
accessibilityLabel: "open user menu: Alexander", | |||
closeMenuAccessibilityLabel: "close menu", | |||
loginSettingsLabel: "Login Settings", | |||
logoutLabel: "Log out", | |||
logoutLabel: "Log out", | |||
type: "standard", |
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.
Remove this since the 'standard' is not a type. it fixes the build if you remove this
@@ -339,6 +340,7 @@ Set the profile menu's `logged-in` property to `true` or `false` to switch betwe | |||
loginHelpLabel: "Help logging in", | |||
registerHeadline: "Need an account?", | |||
registerLabel: "Register now", | |||
type: "standard", |
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.
Remove this since the 'standard' is not a type. it fixes the build if you remove this
@@ -108,6 +108,7 @@ export const TemplateProfileMenu = (args, { argTypes }) => ({ | |||
:login-settings-url="loginSettingsUrl" | |||
:hide-login-settings="hideLoginSettings" | |||
:logout-url="logoutUrl" | |||
:type="type" |
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.
Remove this since the 'standard' is not a type. it fixes the build if you remove this
…in storybook