Skip to content

Conversation

ShimiSun
Copy link
Collaborator

@ShimiSun ShimiSun commented Oct 31, 2022

What does this PR do?

Fixes the zIndex type for drop on the theme object

Where should the reviewer start?

base.d.ts

What testing has been done on this PR?

local

How should this be manually tested?

TS storybook

Do Jest tests follow these best practices?

N/A

Any background context you want to provide?

What are the relevant issues?

Not sure

Screenshots (if appropriate)

Do the grommet docs need to be updated?

No

Should this PR be mentioned in the release notes?

Yes, TS fix

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

backwards compatible

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.

Should we add number to the other places in base.d.ts that use zIndex. I think if we change it in one place it would make sense to change it in the other places as well. I think there are only three other places zIndex is defined in base.d.ts

@ShimiSun
Copy link
Collaborator Author

ShimiSun commented Nov 7, 2022

Should we add number to the other places in base.d.ts that use zIndex. I think if we change it in one place it would make sense to change it in the other places as well. I think there are only three other places zIndex is defined in base.d.ts

I didn't check other places of this prop, their use cases, or how critical their change would be. I fixed this one because it was breaking locally and it is supported for no TS users.

@britt6612
Copy link
Collaborator

Should we add number to the other places in base.d.ts that use zIndex. I think if we change it in one place it would make sense to change it in the other places as well. I think there are only three other places zIndex is defined in base.d.ts

I didn't check other places of this prop, their use cases, or how critical their change would be. I fixed this one because it was breaking locally and it is supported for no TS users.

That's fair we can go back and assess the other area but do not want to block you on this one.

@britt6612 britt6612 self-requested a review November 7, 2022 20:23
@ericsoderberghp ericsoderberghp merged commit e7255df into master Nov 7, 2022
@ericsoderberghp ericsoderberghp deleted the fix/drop/zindex-type branch November 7, 2022 20:49
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