Skip to content

Conversation

@ktabors
Copy link
Member

@ktabors ktabors commented Jul 27, 2021

no issue, but related to #1094 to making testing #2057 with chromatic better

adding Arabic, Chinese, and Japanese strings and locales to Chromatic tests to make sure everything with the improved font-family handling is better.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

added Chinese, Japanese, and Arabic strings to button, menu, and textfield stories within chromatic

🧢 Your Project:

RSP

@adobe-bot
Copy link

Build successful! 🎉

solimant
solimant previously approved these changes Aug 4, 2021
Copy link
Collaborator

@solimant solimant 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! Just minor comments on translation.

ktabors and others added 2 commits August 4, 2021 18:47
Co-authored-by: solimant <solimant@users.noreply.github.com>
Co-authored-by: solimant <solimant@users.noreply.github.com>
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

Just one thing, but otherwise looks good

Comment on lines 152 to 162
export const TemplateArabicWithIcons = (): Story<SpectrumMenuTriggerProps> => (args) => (
<MenuTrigger {...args} isOpen>
<ActionButton>
Menu Button
</ActionButton>
<Menu items={withArabic}>
{item => customMenuItem(item)}
</Menu>
</MenuTrigger>
);

Copy link
Member

Choose a reason for hiding this comment

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

This one isn't rendering in the MenuTrigger chromatic story. Instead of exporting this and importing it in the RTL chromatic story you could do the following:

const TemplateArabicWithIcons = (): Story<SpectrumMenuTriggerProps> => (args) => (
  <MenuTrigger {...args} isOpen>
    <ActionButton>
      Menu Button
    </ActionButton>
    <Menu items={withArabic}>
      {item => customMenuItem(item)}
    </Menu>
  </MenuTrigger>
);
...
export const TemplateArabic = TemplateArabicWithIcons().bind({});
TemplateArabic.storyName = 'Arabic complex items';

and in MenuTriggerRTL file:

export {
  TemplateArabic,
  Default,
  WithSections,
  Complex,
  AlignEnd,
  DirectionTop,
  DirectionBottom,
  DirectionStart,
  DirectionStartEnd,
  DirectionEnd,
  DirectionLeft,
  DirectionLeftEnd,
  DirectionRight,
  DirectionRightEnd
} from './MenuTrigger.chromatic';

@adobe-bot
Copy link

Build successful! 🎉

LFDanLu
LFDanLu previously approved these changes Aug 16, 2021
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

LGTM

@adobe-bot
Copy link

Build successful! 🎉

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

TextArea:value: 測試, icon: Info, labelPosition: side, validationState: valid
doesn't quite fit
nor does
Textfield:value: 測試, icon: Info, labelPosition: side, validationState: valid
I know we have a few that don't fit already, but I want to try to not introduce new ones

The rest look fine

@adobe-bot
Copy link

Build successful! 🎉

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

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

LGTM, the wrap looks good on the textarea/field chromatic stories

@adobe-bot
Copy link

Build successful! 🎉

@snowystinger snowystinger merged commit e0d42c4 into main Sep 15, 2021
@snowystinger snowystinger deleted the chromatic_ar_AE_values branch September 15, 2021 19:19
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.

6 participants