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

feat: add prefix to UiListItem #469

Merged
merged 16 commits into from
May 8, 2024
Merged

feat: add prefix to UiListItem #469

merged 16 commits into from
May 8, 2024

Conversation

pspaczek
Copy link
Contributor

@pspaczek pspaczek commented Apr 22, 2024 •
edited
Loading

Description

This PR:

  • add the UiListItemAffix component as the base for UiListItemPrefix and UiListItemSuffix
  • use UiListItemAffix to create UiListItemPrefix
  • refactor UiListItemSuffix. Use UiListItemAffix and handle @deprecated props icon to keep transparent API for prefix and suffix component
  • handle hover and active states of affix
  • update stories, and adjust them to UiListItem design

Related Issue

Closes #

Motivation and Context

One design of Default UiMenuItem has a prefix element. Same as the design for UiListItem "32 Icon List Item". To allow the use both designs I add the prefix to UiListItem
UiListItem
Zrzut ekranu 2024-04-22 o 12 36 44
UiMenuItem
Zrzut ekranu 2024-04-22 o 12 37 21

How Has This Been Tested?

Null

Screenshots (if appropriate):

Zrzut ekranu 2024-04-25 o 10 36 09
Zrzut ekranu 2024-04-25 o 10 36 20
Zrzut ekranu 2024-04-25 o 10 36 36
Zrzut ekranu 2024-04-25 o 10 39 08

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@pspaczek
refactor: improved performance of UiList (infermedica#434)
ba9b2c6 
* refactor: improved performance of UiList

* feat: style for AsCondition stories

* feat: style for IconInHeading stories

* fix: stories

* fix: stories

* chore: cleaning

* fix: stories

* chore: remove changes from UiListItemSuffixAsButton.vue

* fix: ts errors

* fix: warnings

* fix: ts errors

* fix: lint errors

* refactor: long list stories improvement

* fix: eslint errors

* fix: indentation
@pspaczek pspaczek requested a review from a team as a code owner April 22, 2024 10:41
@pspaczek pspaczek force-pushed the feat/add-prefix-to-ui-list branch from 1660c0e to 8c846da Compare April 22, 2024 11:01
@pspaczek pspaczek mentioned this pull request Apr 23, 2024
Merged
6 tasks
AlanLes
AlanLes reviewed Apr 25, 2024
View reviewed changes
Copy link
Contributor

@AlanLes AlanLes left a comment

Choose a reason for hiding this comment

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

Overall looks good 👍
Have you checked it locally with the MGP? Does it break the app or doesn't it?

src/components/organisms/UiList/_internal/UiListItemSuffix.vue Outdated Show resolved Hide resolved
src/components/organisms/UiList/_internal/UiListItemPrefix.vue Outdated Show resolved Hide resolved
@pspaczek
Copy link
Contributor Author

Overall looks good 👍 Have you checked it locally with the MGP? Does it break the app or doesn't it?

tested in MGP, it doesn't break app

@pspaczek pspaczek enabled auto-merge (squash) April 26, 2024 11:53
@pspaczek pspaczek requested a review from AlanLes April 26, 2024 11:53
kubajmarek
kubajmarek reviewed Apr 26, 2024
View reviewed changes
src/components/organisms/UiList/_internal/UiListItemPrefix.vue
Copy link
Member

Choose a reason for hiding this comment

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

Taking a quick look at ItemPrefix and ItemSuffix, aren't they pretty much identical? The only difference I could find is the reorder of icon & label. 🤔

Copy link
Contributor Author

@pspaczek pspaczek May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yes, I thought about creating an affix component... but finally, I made 2 different components. Give me a minute. I will do that.

@pspaczek pspaczek force-pushed the feat/add-prefix-to-ui-list branch from 8b85a06 to 9ebc990 Compare May 6, 2024 07:22
@pspaczek pspaczek force-pushed the feat/add-prefix-to-ui-list branch from 8527e85 to e86f7b0 Compare May 6, 2024 08:55
@pspaczek pspaczek requested a review from kubajmarek May 6, 2024 09:02
AlanLes
AlanLes reviewed May 6, 2024
View reviewed changes
Copy link
Contributor

@AlanLes AlanLes left a comment

Choose a reason for hiding this comment

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

It could be simplified even more. The Prefix and Suffix are still almost identical, with slight differences in the template. We could get rid of UiListItemPrefix and UiListItemSuffix and use the UiListItemAffix straight away in the UiList, couldn't we? The only difference between those two is the elements' order: icon, label vs. label, icon

@pspaczek
Copy link
Contributor Author

pspaczek commented May 6, 2024

It could be simplified even more. The Prefix and Suffix are still almost identical, with slight differences in the template. We could get rid of UiListItemPrefix and UiListItemSuffix and use the UiListItemAffix straight away in the UiList, couldn't we? The only difference between those two is the elements' order: icon, label vs. label, icon

Order and CSS Variables for both i.e --list-item-item-suffix-icon-color. We can't merge it to --ui-list-item-affix

@pspaczek
Copy link
Contributor Author

pspaczek commented May 6, 2024

@AlanLes now duplicate parts are in the affix component. For Prefix just add template and CSS Variables for suffixes and prefixes. The suffix has @deprecated props. This way allows us to make changes in the template in the feature.

sonya0504
sonya0504 previously approved these changes May 7, 2024
View reviewed changes
pspaczek
pspaczek commented May 7, 2024
View reviewed changes
src/components/organisms/UiMultipleChoices/_internal/UiMultipleChoicesItem.vue Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adjustment to changes in UiListItem styles

@pspaczek pspaczek dismissed sonya0504’s stale review May 7, 2024 09:46

The merge-base changed after approval.

@pspaczek
Merge branch 'refs/heads/develop' into feat/add-prefix-to-ui-list
7985eb9 
# Conflicts:
#	src/components/organisms/UiList/UiList.stories.ts
#	src/components/organisms/UiList/_internal/UiListItem.vue
#	src/components/organisms/UiList/_internal/UiListItemSuffix.vue
#	src/components/organisms/UiList/stories/Condition.vue
#	src/components/organisms/UiList/stories/IconInHeading.vue
@pspaczek pspaczek requested review from sonya0504 and AlanLes May 7, 2024 12:17
pspaczek
pspaczek commented May 7, 2024
View reviewed changes
src/components/organisms/UiMultipleChoices/UiMultipleChoices.vue Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adjustment to changes in UiListItem styles

src/components/organisms/UiMultipleAnswer/_internal/UiMultipleAnswerItem.vue Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adjustment to changes in UiListItem API

src/components/organisms/UiList/_internal/UiListItemAffix.vue Outdated
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 prefer to use "Component Composition" instead of handling templates (and more) in the UiListItemAffix component.

AlanLes
AlanLes approved these changes May 8, 2024
View reviewed changes
Copy link
Contributor

@AlanLes AlanLes left a comment

Choose a reason for hiding this comment

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

OK, nothing more to add ;)

@pspaczek pspaczek merged commit 2ddc9f8 into infermedica:develop May 8, 2024
1 check passed
@pspaczek pspaczek deleted the feat/add-prefix-to-ui-list branch May 8, 2024 10:21
@pspaczek pspaczek mentioned this pull request May 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlanLes AlanLes AlanLes approved these changes

@sonya0504 sonya0504 sonya0504 approved these changes

@kubajmarek kubajmarek Awaiting requested review from kubajmarek

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
@pspaczek @AlanLes @kubajmarek @sonya0504