Skip to content
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

fix(react-keytips): address issues after bug bash #301

Merged

Conversation

mainframev
Copy link
Contributor

@mainframev mainframev commented Feb 1, 2025

Fixing the issues spotted during the bug bash session: 1, 2, 3, 4, 7

@mainframev mainframev requested a review from a team as a code owner February 1, 2025 22:09
@mainframev mainframev self-assigned this Feb 1, 2025
@mainframev mainframev force-pushed the docs/react-keytips-bug-bash-improve branch 5 times, most recently from bea7ae1 to 60fb46b Compare February 2, 2025 19:09
import { addons } from '@storybook/manager-api';

addons.setConfig({
enableShortcuts: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

disabled the shortcuts, as some of them were conflicting with keytips hotkeys. For instance, if invokeEvent is keyup, digit 1 is not captured at all.

@ValentinaKozlova ValentinaKozlova self-requested a review February 3, 2025 15:37
textAlign: 'center',
cursor: 'default',
fontFamily: tokens.fontFamilyBase,
fontSize: tokens.fontSizeBase200,
lineHeight: tokens.lineHeightBase200,
borderRadius: tokens.borderRadiusMedium,
padding: tokens.spacingHorizontalXS,
border: `1px solid ${tokens.colorTransparentStroke}`,
padding: `calc(${tokens.spacingHorizontalXS} - 1px)`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the -1px? Is the box-sizing: border-box not enough?

Copy link
Contributor Author

@mainframev mainframev Feb 4, 2025

Choose a reason for hiding this comment

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

box-sizing is already applied. I add this calc in order to keep the correct token and avoid setting height, just rely on border and padding sizes. The size should be 24x24, which becomes 24x26 with padding token from Figma spec and 1px border, because border is not taking into account in Figma, but we need it, so we could set padding 3px (no token), use fixed height, or this approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should probably use tokens.strokeWidthThin instead of -1px, e.g.:

calc(${tokens.spacingHorizontalXS} - ${tokens.strokeWidthThin})

@mainframev mainframev force-pushed the docs/react-keytips-bug-bash-improve branch from 77bcaad to 62479a0 Compare February 10, 2025 11:40
@mainframev mainframev force-pushed the docs/react-keytips-bug-bash-improve branch from 62479a0 to f6e563d Compare February 18, 2025 19:43
@mainframev mainframev merged commit 6f13f82 into microsoft:main Feb 18, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants