Skip to content

Conversation

AnshumanPadiya
Copy link
Contributor

@AnshumanPadiya AnshumanPadiya commented Oct 6, 2022

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.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent.
  • 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)

themeModeStory

authoThemeMode2

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?

added themeMode story in storbook
Copy link
Contributor

@ericsoderberghp ericsoderberghp left a 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.

@@ -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';
Copy link
Contributor

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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<Box pad="medium">Check New Theme Mode</Box>
<Box pad="medium">"auto" themeMode</Box>

updated themeMode storybook
removed unwanted comment from Grommet.js
Copy link
Collaborator

@jcfilben jcfilben left a 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?

@AnshumanPadiya
Copy link
Contributor Author

@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.

@jcfilben
Copy link
Collaborator

@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?

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.

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.

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...
If you are interested in making Grommet more accessible see issue 6213 or filter issues by the accessibility label

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.

@ericsoderberghp ericsoderberghp merged commit 69f44bc into grommet:master Oct 12, 2022
@AnshumanPadiya
Copy link
Contributor Author

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.

@jcfilben
Copy link
Collaborator

Awesome! Thanks for making the theme pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants