-
-
Notifications
You must be signed in to change notification settings - Fork 1
Feature/dropdown #3
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
base: master
Are you sure you want to change the base?
Conversation
Byloth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ti ho lasciato un po' di commenti.
Correggi - correttamente - la PR prima ch'io esegua il merge.
| import ClayDropdown from "./ClayDropdown.vue"; | ||
| import type { Meta, StoryObj } from "@storybook/vue3"; | ||
|
|
||
| const meta: Meta<typeof ClayDropdown> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Utilizza la stessa struttura degli altri file.
Definisci un'interfaccia con quelle che sono le proprietà da gestire.
| export default meta; | ||
| type Story = StoryObj<typeof meta>; | ||
|
|
||
| export const Default: Story = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gestisci la storia come quella degli altri file.
Aggiungi il metodo render e il template in cui vengono settate le proprietà correttamente.
| } | ||
| }; | ||
|
|
||
| export const WithSelection: Story = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non è una variante.
| } | ||
| }; | ||
|
|
||
| export const WithLongText: Story = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avrebbe - anche - avuto senso come variante se ci fossero state delle differenze a livello di stile o di classi...
Ma se l'unica differenza è la lunghezza del testo delle opzioni, non è una variante.
src/components/ClayDropdown.vue
Outdated
| modelValue?: string; | ||
| } | ||
|
|
||
| const props = withDefaults(defineProps<Props>(), { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alcuni suggeriscono di usare il defineProps wrappato dal metodo withDefault.
Io non sono d'accordo: mi sembra solo una "soluzione" ad un problema che - in realtà - non esiste.
In Vue le proprietà si possono definire in una sola chiamata, senza definire alcuna interfaccia extra, usando correttamente il metodo defineProps.
Guarda negli altri file come funziona.
src/components/ClayDropdown.vue
Outdated
| const selectItem = (item: { label: string; value: string }) => | ||
| { | ||
| selectedValue.value = item.value; | ||
| emit("update:modelValue", item.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Qui - di conseguenza - usando il defineModel non dovrai emettere più alcun evento.
Ti basterà modificarne il valore:
value.value = item.value;Ricordati di rimuovere le proprietà e le variabili che non saranno più utilizzate.
src/components/ClayDropdown.vue
Outdated
| $dropdown-color-background-dark: #2c2e30; | ||
| $dropdown-color-background-hover-dark: rgba(42, 42, 42, 0.9); | ||
| $dropdown-color-outline-dark: color.complement($dropdown-color-background-dark); | ||
| $dropdown-color-shadow-dark: color.adjust($dropdown-color-background-dark, $lightness: -30%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le variabili SCSS non devono più essere usate.
src/components/ClayDropdown.vue
Outdated
|
|
||
| @media (prefers-color-scheme: dark) { | ||
| .clay-dropdown { | ||
| background-color: var(--clay-dropdown-color-background-dark); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le variabili CSS servono per poter essere usate anche in maniera dinamica.
Se io dovessi definire:
:root
{
--clay-dropdown-color-background: white;
--clay-dropdown-shadow: inset 0 2px 5.1px 0 rgba(0, 0, 0, 0.25),
0 2px 4px 0 rgba(142, 138, 138, 0.75);
}
.clay-dropdown
{
background-color: var(--clay-dropdown-color-background);
box-shadow: var(--clay-dropdown-shadow);
&:focus
{
--clay-dropdown-shadow: inset 0 2px 5.1px 0 rgba(0, 0, 0, 0.35),
0 2px 4px 0 rgba(142, 138, 138, 0.85);
}
}
@media (prefers-color-scheme: dark)
{
:root
{
--clay-dropdown-color-background: black;
--clay-dropdown-shadow: inset 0 3px 8px 0 rgba(0, 0, 0, 0.6),
0 3px 6px 0 rgba(0, 0, 0, 0.4);
}
.clay-dropdown
{
&:focus
{
--clay-dropdown-shadow: inset 0 4px 10px 0 rgba(0, 0, 0, 0.7),
0 4px 8px 0 rgba(0, 0, 0, 0.5);
}
}
}Il colore di background cambierebbe correttamente, senza risovrascrivere alcuna regola CSS.
Stessa cosa accadrebbe per la shadow, che cambierebbe correttamente.
Correggi l'uso di tutte questa variabili.
Prendi spunto dagli altri file già fatti; lì è già stato implementato correttamente.
| --clay-dropdown-transition-duration-instant: 0.1s; | ||
| --clay-dropdown-transition-timing: cubic-bezier(0.4, 0, 0.2, 1); | ||
| --clay-dropdown-transition-timing-out: ease-out; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ho il vago sentore che molte di queste variabili CSS non dovrebbero aver ragione di esistere.
Controlla il commento precedente.
No description provided.