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

fix: use color and variant for AListItem #87

Closed
wants to merge 13 commits into from

Conversation

ManUtopiK
Copy link
Contributor

This change add the possibility to use layerProps (color and variant) to AListItem with default to text.
If color is provided with variant, is-active item use this variant and hovered item use light variant.

...
<AListItem color="danger" title="Delete" />
...

image

// docs/demos/list/DemoListVModelSupport.vue#L39
...
<AListItem
    v-for="(item, index) in items"
    color="info"
    variant="fill"
...

image

TODO :

  • update doc

@netlify
Copy link

netlify bot commented Nov 30, 2022

Deploy Preview for anu-vue failed.

Name Link
🔨 Latest commit 689d2df
🔍 Latest deploy log https://app.netlify.com/sites/anu-vue/deploys/639943a93d00c60008b5cc5e

@ManUtopiK
Copy link
Contributor Author

Is this change acceptable ? I write the doc ?

@jd-solanki
Copy link
Owner

jd-solanki commented Dec 9, 2022

Is this change acceptable ? I write the doc ?

I will review it soon

toRef(props, 'states'),
{ statesClass: 'states:10' },
computed(() => ({ statesClass: props.isActive ? 'states:10' : 'hover:states:10' })),
Copy link
Owner

Choose a reason for hiding this comment

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

@ManUtopiK What does this line do?

Copy link
Owner

Choose a reason for hiding this comment

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

I mean what's the need for hover:states:10'?

Copy link
Contributor Author

@ManUtopiK ManUtopiK Dec 14, 2022

Choose a reason for hiding this comment

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

I don't remember exactly why it was for. But I struggled a little bit with states before #86.

Also, there are still some improvements to do.

  1. We lose the state of the isActive item when using together a color AND the variant text.

Basic example without color and text:

image

With props color="success" variant="text":

image
Donut jujubes isn't highlighted.

  1. We can't add color when there is no click handler.
<AListItem
  title="Send"
  color="success"
/>

color props do nothing here.

@ManUtopiK
Copy link
Contributor Author

Arff, I'm really sorry for the big mess I made on this branch. I wanted to update and use #86 but I made a mistake.
I don't understand the state here on gitub. My branch seems good ; 3 commits ahead jd-solanki:main.
Git is hard :/

It's late. I will close this pull request and create a new one tomorrow...
Sorry again for the noise.

@ManUtopiK
Copy link
Contributor Author

I close this pull request for now. I broke the branch anyway.
I have to think on component state and variant to go further.

@ManUtopiK ManUtopiK closed this Dec 14, 2022
@jd-solanki
Copy link
Owner

Hi @ManUtopiK

I am refactoring the whole codebase from TSX to SFC so if want to create a new PR, please wait for 1 or 2 days to get latest updates.

@ManUtopiK
Copy link
Contributor Author

I am refactoring the whole codebase from TSX to SFC

Woah, a lot of changes ! I've worked on a loading component. I didn't finish. I'm back after Christmas.

@jd-solanki
Copy link
Owner

jd-solanki commented Dec 20, 2022

We are just moving from TSX to SFC, Transition will be easier. We just have to convert the code. Your implementation logic will stay the same.

Here are the benefits I found so far ❤️
https://discord.com/channels/1028973058649751602/1028977624569086022/1053911768902160436

One of the most important is #15

I will finish refactoring in maybe 1-2 days.

Merry Christmas, Buddy 🥳

@jd-solanki
Copy link
Owner

Hi @ManUtopiK refactoring PR is merged! 😇

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.

None yet

3 participants