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.

Sign up for GitHub

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

Jump to bottom

LPD-46157 Create Icon Selector (No Keyboard Interactions) #5929

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

LPD-46157 Create Icon Selector (No Keyboard Interactions) #5929

wants to merge 7 commits into from

Conversation

ilzamcmed
Copy link
Member

@ilzamcmed ilzamcmed commented Jan 27, 2025 •
edited
Loading

https://liferay.atlassian.net/browse/LPD-46157

simplescreenrecorder-2025-01-27_17.05.24.mp4

@ilzamcmed ilzamcmed changed the title Create Icon Selector (No Keyboard Interactions) LPD-46157 Create Icon Selector (No Keyboard Interactions) Jan 27, 2025
@ilzamcmed
Copy link
Member Author

Hey @matuzalemsteles, this is the inicial commits for the icon selector component.

@ilzamcmed ilzamcmed force-pushed the LPD-46157 branch 4 times, most recently from 037661f to f5ec448 Compare January 28, 2025 18:09
ethib137
ethib137 requested changes Jan 30, 2025
View reviewed changes
Copy link
Member

@ethib137 ethib137 left a comment

Choose a reason for hiding this comment

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

Thanks Ilza!

matuzalemsteles
matuzalemsteles reviewed Feb 7, 2025
View reviewed changes
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.

Great work here, I just added some comments so we can readjust the component.

@ilzamcmed ilzamcmed force-pushed the LPD-46157 branch 3 times, most recently from 9338480 to 3def2fd Compare February 11, 2025 14:45
@ilzamcmed
Copy link
Member Author

Hey @ethib137 and @matuzalemsteles,
I've send some of the changes you guys requested but I'm still working on the useOverlayPosition change to use as an enum and on the usage of useResource for fetchIcons

@ilzamcmed ilzamcmed marked this pull request as ready for review February 13, 2025 16:55
@ilzamcmed ilzamcmed force-pushed the LPD-46157 branch 2 times, most recently from 6a25c8c to fafd272 Compare February 13, 2025 17:39
@ethib137
Copy link
Member

@matuzalemsteles can you please review these changes again?

matuzalemsteles
matuzalemsteles reviewed Feb 18, 2025
View reviewed changes
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.

Overall, it seems ok, but I noticed two things that haven't been done yet. About the use of useResource as I mentioned in Slack, maybe it should work now. If it doesn't work, we can schedule a meeting.

I saw that we haven't moved the IconSelectTrigger component into the IconSelector component yet, to simplify things a bit more and allow the developer to configure the component.

The documentation will also need to be updated to the new standard we have now. We won't need to change much, just the way we write the examples, which is simpler now.

ethib137
ethib137 reviewed Feb 18, 2025
View reviewed changes
packages/clay-css/src/scss/_components.scss Outdated
@@ -16,6 +16,7 @@
@import 'components/_toasts';

@import 'components/_icons';
@import 'components/_icon-selector';
Copy link
Member

Choose a reason for hiding this comment

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

@pat270 were should this be placed in this list and why? Can you give a short explanation of the logic of the order of this file?

Copy link
Member

Choose a reason for hiding this comment

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

@pat270 ^

@ethib137 ethib137 requested review from pat270 and ethib137 February 18, 2025 06:01
@ilzamcmed ilzamcmed force-pushed the LPD-46157 branch 2 times, most recently from 0337f02 to 58c19b7 Compare February 19, 2025 12:13
@matuzalemsteles
Copy link
Member

@ilzamcmed I'm making some adjustments to the component, I'll push some incremental commits tomorrow. It seems that useResource is working fine, I didn't get any error messages.

@ilzamcmed
Copy link
Member Author

Thanks @matuzalemsteles.
I'll go over the figma to see if I missed anything related with styles.
Let me know if there's anything else I can do!

@ilzamcmed
Copy link
Member Author

Hey @matuzalemsteles , after the dependencies update, the verify dependencies test stuck at build, it don't move forward. This is happening locally also. Do you have any idea of what's happening?

@matuzalemsteles
Copy link
Member

@ilzamcmed I've made some changes, I've changed the code a lot so it might break something if you're working on it. I've added public APIs so that the developer can configure them and I've kept the component in just one place instead of keeping a function inside the component.

I've removed some APIs that I saw weren't necessary and I've also added the spritemap API. I've realized that in the end we were using the clay-css package instead of the developer being able to configure it, this is important for the portal.

ethib137
ethib137 requested changes Feb 27, 2025
View reviewed changes
Copy link
Member

@ethib137 ethib137 left a comment

Choose a reason for hiding this comment

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

Nice work! Just a couple things.

packages/clay-css/src/scss/_components.scss Outdated
@@ -16,6 +16,7 @@
@import 'components/_toasts';

@import 'components/_icons';
@import 'components/_icon-selector';
Copy link
Member

Choose a reason for hiding this comment

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

@pat270 ^

packages/clay-css/src/scss/components/_icon-selector.scss Outdated
@@ -0,0 +1,20 @@
// Variables
$grid-columns: 9;
$max-input-width: 200px;
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't have a comment like this and we probably don't need variables that are only used once each. @pat270 can you review the css, please?

@ilzamcmed ilzamcmed requested a review from ethib137 February 27, 2025 16:41
@pat270
Copy link
Member

pat270 commented Feb 28, 2025

@ilzamcmed I realized we didn't need to add a new component in Clay CSS after looking through the markup. I added to dropdown and input-group. I went through and fixed a lot of the style inconsistencies since everyone is on holiday there.

icon-selector

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pat270 pat270 pat270 approved these changes

@matuzalemsteles matuzalemsteles Awaiting requested review from matuzalemsteles

@ethib137 ethib137 Awaiting requested review from ethib137

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ilzamcmed @ethib137 @matuzalemsteles @pat270