-
Notifications
You must be signed in to change notification settings - Fork 418
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
Trial Bar: Add component #2025
Trial Bar: Add component #2025
Conversation
Awesome! @davidlygagnon and I will take a look at this PR tomorrow! |
components/trial-bar/button.jsx
Outdated
*/ | ||
style: PropTypes.object, | ||
}; | ||
|
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.
@garygong @davidlygagnon I would recommend non-custom props for TrailBar's Button
and Dropdown
link to the Button and Dropdown doc pages in the comment on this line inside a comment which would be the larger type on the component description. This would allow the removal of the prop table from the docs and decrease duplicate lines of code which may not be updated in the future, for instance, when we add props to dropdown.
Something closer to Radio
on this page https://react.lightningdesignsystem.com/components/radio-groups/
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.
Looks good Pulkit! Just left a few comments. Thanks
const TrialBarDropdown = (props) => { | ||
const { label, ...rest } = props; | ||
return ( | ||
<Dropdown {...rest} inverse> |
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.
Is it really necessary to add a new prop inverse
on dropdown component since it's only adding a class name? Maybe we can just use className
instead?
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 use just the className
but isn't having an inverse
variant of the dropdown worth it?
|
||
// This component accepts the same props as Button. | ||
// eslint-disable-next-line react/forbid-foreign-prop-types | ||
const { propTypes } = Button; |
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 wonder if we really need this and whether it will break people that are using the babel-plugin-transform-react-remove-prop-types
plugin to remove props. Would it be sufficient to just have a link pointing to the Dropdown component?
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 used BuilderHeader
for reference, if that's working for the consumers I don't think this is much different. I will update it if need be.
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.
Ok - let's leave as is then.
import Button from '~/components/button'; | ||
import { TRIAL_BAR_BUTTON } from '../../utilities/constants'; | ||
|
||
// This component accepts the same props as Button. |
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 is a doc site matter, so it's not very obvious, but this comment should follow the way that component descriptions should to be written to be picked up by react-docgen
. I think that requires the comment it to in multi-line quote comment on the line directly before the class/function declaration. A markdown URL to the Button page would be good https://react.lightningdesignsystem.com/components/buttons/ . I think there are other examples in the library. The easiest way to check is to run npm build:docs
and make sure this component shows up.
@davidlygagnon I'd recommend running the doc site consuming this branch locally to make sure all the comments and props show up correctly--or maintainers can fix this up, since the doc site isn't public. Either way is fine. As you know, the whole library/doc site can be finicky. Kevin and I spent some time with Setup Assistant yesterday.
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 have added multi-line comments right before the components declaration but haven't removed the propTypes
. It is as done in BuilderHeader
component that I was using as a reference. Let me know if this needs further updation.
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'm referring to doc comments only. Please see https://github.com/salesforce/design-system-react/blob/master/components/builder-header/nav-dropdown.jsx#L16
which shows up on the doc site here:
Please add a link to the doc site Button there.
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.
Cool - I'll double check by building the doc site locally.
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.
Looks good to me!
Will be merging today.
@pulkonet Before we merge this, we need to fix the docs:
Please update |
@pulkonet I'm still seeing a few console errors on launching the docs site. We need to expose the sub-components of trial-bar as well ( We're almost there! |
@pulkonet I'm ready to merge it now. Doc site shows the component correctly. Can you fix the merge conflicts. Thanks! |
Fixes #2006
Also adds an inverse variant to the dropdown menu component, to work with Trial Bar.
Additional description
CONTRIBUTOR checklist (do not remove)
Please complete for every pull request
npm run lint:fix
has been run and linting passes.components/component-docs.json
CI tests pass (npm test
).REVIEWER checklist (do not remove)
components/component-docs.json
tests.Required only if there are markup / UX changes
last-slds-markup-review
inpackage.json
and push.last-accessibility-review
, topackage.json
and push.npm run local-update
within locally cloned site repo to confirm the site will function correctly at the next release.