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(@clayui/language-picker): LPD-45714 Add security margin #5914

Merged
merged 21 commits into from
Jan 22, 2025

Conversation

veroglez
Copy link
Member

@veroglez veroglez commented Jan 15, 2025

Hi guys!,

I’m sending this pull request where we’re contributing a new component to Clay according to this Epic https://liferay.atlassian.net/browse/LPD-17787.

It’s a Language Picker and it has two variants:

  • A basic language picker.
  • A language picker with translations, where each item in the picker is accompanied by a label indicating the translation status.
Screen.Recording.2025-01-15.at.5.49.42.PM.mov

As shown in the video, both variants support a small size and the option to hide the trigger text.

A new feature in the translation variant is the addition of a label to indicate the progress of a translation. This can be useful in cases where the language selector considers more than one field, such as in a form. Along with the existing labels (Default, Translated, and Untranslated), this progress label is now included.

In the code I have also added all the tests that I think are necessary, if you consider that I have to add any more, do not hesitate to tell me.

Could you review it? Any feedback is welcome.

Thanks in advance! 😄

cc @drakonux

@veroglez veroglez changed the title Lpd 45714 feat(@clayui/language-picker): LPD-45714 Add security margin Jan 15, 2025
@veroglez
Copy link
Member Author

@pat270, Could you help me with the styles of the picker options? It seems that the label is misaligned with the rest of the elements (icon, flag, text)

styles

@pat270
Copy link
Member

pat270 commented Jan 16, 2025

@veroglez Sure I'll take a look

packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
</span>

{!hideTriggerText ? (
<span className="font-weight-normal mr-2">
Copy link
Member

Choose a reason for hiding this comment

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

Without the btn class we won't need font-weight-normal. Do we need mr-2 here because the spacing is incorrect between the arrows and text? I can update in CSS if so.

Copy link
Member Author

@veroglez veroglez Jan 17, 2025

Choose a reason for hiding this comment

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

Yes, that margin is a safety spacing between the text and the arrows. Updating it in CSS would be ideal.
This change would only be for the case where the text is displayed, since the spacing is correct when only the flag is displayed.

Screenshot 2025-01-17 at 4 02 09 PM

@veroglez
Copy link
Member Author

veroglez commented Jan 17, 2025

@pat270 I have updated the code with the changes you recommended for the picker button. Thanks a lot! Do you have any recommendations for the picker options? for flag, text and label alignment?

@pat270
Copy link
Member

pat270 commented Jan 17, 2025

@veroglez I sent a separate pr for the alignment at #5917

Copy link
Member

@matuzalemsteles matuzalemsteles left a comment

Choose a reason for hiding this comment

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

Hi @veroglez, Thanks for working on this. We are avoiding creating new packages for the new components and instead we are adding the new components under the @clayui/core package, can you move this component to clay-core? Also I added some more comments.

packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
packages/clay-language-picker/src/index.tsx Outdated Show resolved Hide resolved
@veroglez
Copy link
Member Author

Hi @veroglez, Thanks for working on this. We are avoiding creating new packages for the new components and instead we are adding the new components under the @clayui/core package, can you move this component to clay-core? Also I added some more comments.

@matuzalemsteles thanks a lot for the review! I updated the code with your suggestions, let me know what you think

Copy link
Member

@matuzalemsteles matuzalemsteles left a comment

Choose a reason for hiding this comment

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

@veroglez thanks for working on this, great job. Just a small tweak you can just export the component in the index.ts in the root package file so people can use import {LanguagePicker} from '@clayui/core';. Just a little detail in these new components we are removing the Clay naming convention from the component names.

@veroglez
Copy link
Member Author

@veroglez thanks for working on this, great job. Just a small tweak you can just export the component in the index.ts in the root package file so people can use import {LanguagePicker} from '@clayui/core';. Just a little detail in these new components we are removing the Clay naming convention from the component names.

Sure! I updated this in b030db1

@matuzalemsteles
Copy link
Member

Going ahead with this and merging. It will be available in the release this week.

@matuzalemsteles matuzalemsteles merged commit f5719b4 into liferay:master Jan 22, 2025
4 checks passed
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