-
Notifications
You must be signed in to change notification settings - Fork 1k
added auto as themeMode prop #6397
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
Conversation
added themeMode story in storbook
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.
Nice start, just a few comments.
src/js/components/Grommet/Grommet.js
Outdated
@@ -23,6 +23,7 @@ import { format, MessageContext } from '../../contexts/MessageContext'; | |||
import defaultMessages from '../../languages/default.json'; | |||
import { GrommetPropTypes } from './propTypes'; | |||
import { AnalyticsProvider } from '../../contexts/AnalyticsContext'; | |||
// import { type } from 'os'; |
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.
Should remove this
|
||
export const ThemeMode = () => ( | ||
<Grommet theme={grommet} themeMode="auto"> | ||
<Box pad="medium">Check New Theme Mode</Box> |
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.
<Box pad="medium">Check New Theme Mode</Box> | |
<Box pad="medium">"auto" themeMode</Box> |
updated themeMode storybook removed unwanted comment from Grommet.js
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! Can you create a pull request on grommet-site to add themeMode="auto"
to the documentation?
@jcfilben Thanks, and sure thing will look into that. I had a couple of questions though if you don't mind answering, do I need to update my branch before it gets merged or will it be fine this way? also could you please suggest to me any other issue at which I could work ( it doesn't have to be included in hacktoberfest ), I would really like to make more contributions to this project. |
No need to update this branch, once it gets reviewed and approved by another person from the grommet team we will update it. If there are any merge conflicts we will let you know and ask you to fix those, but as of right now no action is needed on your end for this branch.
That's awesome that you want to make more contributions to the project! Any pull requests on this repo that get approved and merged will count towards hacktoberfest even if the original issue doesn't have the hacktoberfest label. The 'hacktoberfest' label just gives people a good starting place to look for issues to work on. As far as potential issues to work on it depends what you are interested in... Interested in enhancements? See issue 6370 or filter issues with the enhancement label Interested in typescript? See issue 5599 or filter issues by the typescript label If you come across any issues that you have questions on or are interested in working on but not sure how to approach the issue go ahead and just drop a comment on the issue & feel free to reach out with any additional questions. |
Great will be looking into those issues! Also, I have made a PR for the 'themeMode' docs with the required changes, please look into it as well. |
Awesome! Thanks for making the theme pull request |
What does this PR do?
allows auto to be passed as a value in themeMode prop - issue #6397
Where should the reviewer start?
Line 91 in Grommet/Grommet.js
What testing has been done on this PR?
How should this be manually tested?
added themeMode story in storybook
Do Jest tests follow these best practices?
screen
is used for querying.userEvent
is used in place offireEvent
.asFragment()
is used for snapshot testing.Any background context you want to provide?
The changes are not
What are the relevant issues?
#6397
Screenshots (if appropriate)
Do the grommet docs need to be updated?
No
Should this PR be mentioned in the release notes?
Is this change backwards compatible or is it a breaking change?