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

feat(component): localize components #1200

Merged
merged 15 commits into from
Jul 14, 2023

Conversation

jorgemoya
Copy link
Contributor

@jorgemoya jorgemoya commented Apr 21, 2023

Breaking changes

What?

Localize components per feedback from #1194

How

Created a localization object prop that has all values that can be localized within the component. This is a breaking change because I changed the pagination props.

Screen with example of new localization prop

screencapture-localhost-3000-table-2023-05-10-13_59_58

Testing/Proof

Locally + unit tests.

@jorgemoya jorgemoya force-pushed the localize-components branch 2 times, most recently from b6ada22 to 4865b06 Compare April 28, 2023 14:45
@jorgemoya jorgemoya changed the title Localize components feat(component): localize components Apr 28, 2023
@jorgemoya jorgemoya marked this pull request as ready for review April 28, 2023 15:47
@jorgemoya jorgemoya requested review from a team as code owners April 28, 2023 15:47
@jorgemoya jorgemoya requested a review from bc-andreadao April 28, 2023 15:47
@jorgemoya jorgemoya force-pushed the localize-components branch from 40c026e to d6ddb8d Compare April 28, 2023 18:05
@bc-andreadao
Copy link
Contributor

@jorgemoya For the dev docs team to suggest copyedits, we need some screenshots of the front end changes. Thanks!

@jorgemoya
Copy link
Contributor Author

@jorgemoya For the dev docs team to suggest copyedits, we need some screenshots of the front end changes. Thanks!

Screenshot 2023-05-10 at 1 58 29 PM

This is how it looks for all components (plus/minus the correct type for the corresponding component). Is this enough to 👀 ?

@bc-andreadao
Copy link
Contributor

@jorgemoya For the dev docs team to suggest copyedits, we need some screenshots of the front end changes. Thanks!

Screenshot 2023-05-10 at 1 58 29 PM This is how it looks for all components (plus/minus the correct type for the corresponding component). Is this enough to 👀 ?

Got it, thanks @jorgemoya !

@jorgemoya jorgemoya force-pushed the localize-components branch from d6ddb8d to aaf3a9f Compare May 16, 2023 15:40
@jorgemoya
Copy link
Contributor Author

Hey @funivan 👋 . Eugene suggested I tag you in this PR to look for your opinion on this implementation of adding translation in BD components. Wanted to hear your input on how I did this, if there is a better way that you've seen, and if this works with the translation tools in the CP. I'm open to suggestions!

@funivan
Copy link

funivan commented Jun 13, 2023

@jorgemoya LGTM. I'll ping folks that are working more closely with big-design components.
@icatalina @golcinho @RomanKrasinskyi

Copy link

@RomanKrasinskyi RomanKrasinskyi left a comment

Choose a reason for hiding this comment

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

Nice job 👏.
I left a few comments. Please look at them

renderOptional &&
css`
&::after {
color: ${theme.colors.secondary60};
content: ' (optional)';
content: ' (${optionalLabel?.toLowerCase() ?? 'optional'})';
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't be enforcing this, it is up to the consumer to decide if they want lowercase or uppercase.

@@ -68,7 +72,7 @@ export const InlineMessage: React.FC<InlineMessageProps> = memo(
{props.onClose && (
<GridItem>
<MessagingButton
iconOnly={<CloseIcon size="medium" title="Close." />}
Copy link
Contributor

Choose a reason for hiding this comment

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

we lost the .?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's okay to remove it. I think the initial intention was to create a pause when a screen reader lands on the button but it should occur naturally since a SR will naturally stop on the element.

);
}, [actions]);
export const ButtonGroup: React.FC<ButtonGroupProps> = memo(
({ actions, localization, ...wrapperProps }) => {
Copy link
Contributor

@icatalina icatalina Jun 14, 2023

Choose a reason for hiding this comment

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

Can we define a default localization object instead of adding the translations adhoc?

Something like:

const defaultLocalization: Localization {
  more: 'more',
};

// ...

export const ButtonGroup: React.FC<ButtonGroupProps> = memo(
  ({ actions, localization = defaultLocalization, ...wrapperProps }) => {

// ...

      iconOnly={<MoreHorizIcon title={localization.more} />}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it this way at some point and reverted to what I have now 😅 . In other words, I agree we this!

optional: string;
}

const defaultLocalization = {
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
const defaultLocalization = {
const defaultLocalization: Localization = {

totalItems: number,
of = 'of',
): string => {
const defaultGetRangeLabel = (start: number, end: number, totalItems: number): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add this as part of the localization object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like having the function be part of the object? My concern is that this getRangeLabel can be used for non localization purposes.

@jorgemoya jorgemoya force-pushed the localize-components branch from 2d92b3d to 210faac Compare June 14, 2023 21:23
renderOptional &&
css`
&::after {
color: ${theme.colors.secondary60};
content: ' (optional)';
content: ' (${localization?.optional})';
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 if localization is nullable, that means localization?.optional can be undefined. Are types correct?

@@ -62,6 +85,8 @@ const InternalStatefulTable = <T extends TableItem>({
itemName,
items = [],
keyField,
localization: localizationProp,
Copy link
Contributor

Choose a reason for hiding this comment

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

might want to keep the style across the change:

Suggested change
localization: localizationProp,
localization = defaultLocalization,

@jorgemoya jorgemoya force-pushed the localize-components branch 3 times, most recently from 35f15c5 to 393e7bc Compare July 6, 2023 19:05
@jorgemoya jorgemoya requested a review from RomanKrasinskyi July 7, 2023 02:06
@jorgemoya
Copy link
Contributor Author

Added the fixes, please review if you have time. 🙏

@@ -68,7 +72,7 @@ export const InlineMessage: React.FC<InlineMessageProps> = memo(
{props.onClose && (
<GridItem>
<MessagingButton
iconOnly={<CloseIcon size="medium" title="Close." />}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's okay to remove it. I think the initial intention was to create a pause when a screen reader lands on the button but it should occur naturally since a SR will naturally stop on the element.

{
name: 'getRangeLabel',
types: '(start: number, end: number, totalItems: number) => string',
required: false,
description: 'A callback to format the label of the per-page range dropdown.',
},
{
name: 'localization',
types: '{of: string, previousPage: string, nextPage: string}',
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
types: '{of: string, previousPage: string, nextPage: string}',
types: '{ of: string, previousPage: string, nextPage: string }',

@jorgemoya jorgemoya force-pushed the localize-components branch from a06789d to 4683dae Compare July 13, 2023 20:49
@jorgemoya jorgemoya merged commit 151484b into bigcommerce:master Jul 14, 2023
@jorgemoya jorgemoya deleted the localize-components branch July 14, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants