Skip to content

Conversation

ShimiSun
Copy link
Collaborator

What does this PR do?

Supports the theme's focus.shadow to accept a string.

Where should the reviewer start?

theme file with types

What testing has been done on this PR?

local tests.

How should this be manually tested?

locally.

Do Jest tests follow these best practices?

N/A

Any background context you want to provide?

On src/js/utils/styles.js we use:

return `
      outline: none;
      box-shadow: ${focus.shadow};
    `;

and since box-shadow accepts the type of string, it should be accepted as a valid input type as well.

What are the relevant issues?

#5446

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Where are the global theme types are documented? the new type should be added there.

Should this PR be mentioned in the release notes?

yep, as a TS fix.

Is this change backward compatible or is it a breaking change?

backward compatible

@jcfilben
Copy link
Collaborator

Do the grommet docs need to be updated?
Where are the global theme types are documented? the new type should be added there.

Some of the global theme props are documented on the grommet theme wiki but this is out of date and doesn't include the latest props. I am working on creating a global theme props page on grommet-site that will include documentation for all of the global theme props.

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!

@jcfilben jcfilben requested a review from ericsoderberghp July 18, 2022 19:12
@ericsoderberghp ericsoderberghp merged commit b7d4623 into master Jul 18, 2022
@ericsoderberghp ericsoderberghp deleted the fix/type-focus-shadow branch July 18, 2022 20:04
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.

3 participants