alert toast component #248
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
| import type { AlertInterface, AlertOptions, AlertType } from './types'; | ||
| import { getId } from './constant'; | ||
|
|
||
| const useStore = createSharedComposable(() => { |
There was a problem hiding this comment.
I'd suggest to get rid of this composable and just use a ref at the useAlerts
const alerts = ref<Alert[]>([]);There was a problem hiding this comment.
I think Composable keeps the refs' state, ref has being moved
iamgabrielsoft
requested review from
e11sy
and removed request for
TatianaFomina and
elizachi
|
|
||
| &--bottom-center { | ||
| left: 50%; | ||
| bottom: 2rem |
| } | ||
|
|
||
| &--default { | ||
| background-color: var(--base--solid-secondary); |
There was a problem hiding this comment.
in default it should be --base--bg-secondary
| /** | ||
| * position of alert, default position -> bottom-center | ||
| */ | ||
| export enum POSITION { | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| BOTTOM_CENTER = 'bottom-center' | ||
| } |
There was a problem hiding this comment.
lets get rid of this param as it is not used at the moment
| timeout?: number; | ||
| } | ||
|
|
||
| export interface AlertInterface { |
There was a problem hiding this comment.
lets move it to the useAlert file and rename to the useAlertComposableState
| .fade-enter-from, | ||
| .fade-leave-to { | ||
| opacity: 0; | ||
| transform: translateY(30px); |
There was a problem hiding this comment.
lets use simple fade-out effect for leaving
| transform: translateY(30px); |
There was a problem hiding this comment.
please look at the updated one, if its good enough
| <style lang="postcss"> | ||
| .fade-enter-active, | ||
| .fade-leave-active { | ||
| transition: opacity 0.5s ease, transform 0.5s ease; |
There was a problem hiding this comment.
| transition: opacity 0.5s ease, transform 0.5s ease; | |
| transition: opacity 0.25s ease, transform 0.25s ease; |
| error: (opt?: Pick<AlertOptions, 'icon' | 'message' | 'timeout'>) => triggerAlert('error', opt), | ||
| warning: (opt?: Pick<AlertOptions, 'icon' | 'message' | 'timeout'>) => triggerAlert('warning', opt), | ||
| info: (opt?: Pick<AlertOptions, 'icon' | 'message' | 'timeout'>) => triggerAlert('info', opt), | ||
| defaultAlert: (opt?: Pick<AlertOptions, 'icon' | 'message' | 'timeout'>) => triggerAlert('default', opt), |
There was a problem hiding this comment.
lets rename defaultAlert to alert
| /** | ||
| * Various alert type | ||
| */ | ||
| export type Alertype = 'success' | 'error' | 'warning' | 'info' | 'default'; |
There was a problem hiding this comment.
| export type Alertype = 'success' | 'error' | 'warning' | 'info' | 'default'; | |
| export type AlertType = 'success' | 'error' | 'warning' | 'info' | 'default'; |
| position: 'bottom-center', | ||
| message: '', | ||
| icon: '', | ||
| type: undefined, |
There was a problem hiding this comment.
lets make it default by default
There was a problem hiding this comment.
as in defaultProps for the variable name?
| id: genId(), | ||
| position: 'bottom-center', | ||
| content: '', | ||
| icon: '', | ||
| type: undefined, | ||
| timeout: 5000, |
There was a problem hiding this comment.
it's better to leave the only place with default values. I think this properties are note related to Alerts Container
There was a problem hiding this comment.
probably,alerts returned by useAlert() should include them
There was a problem hiding this comment.
it's better to leave the only place with default values. I think this properties are note related to Alerts Container
its used in Alert container position
There was a problem hiding this comment.
probably,
alertsreturned byuseAlert()should include them
yes I added a defaultAlertOpt on useAlert()
| if (props.type === 'success') { | ||
| return 'grass'; | ||
| } | ||
|
|
||
| if (props.type === 'error') { | ||
| return 'red'; | ||
| } | ||
|
|
||
| if (props.type === 'warning') { | ||
| return 'amber'; | ||
| } | ||
|
|
||
| if (props.type === 'info') { | ||
| return 'graphite'; | ||
| } |
There was a problem hiding this comment.
use switch case for that please
| /** | ||
| * Type of the alert. | ||
| * | ||
| * Can be any of `(success, error, default, info, warning)` | ||
| */ | ||
| type?: Alertype; |
There was a problem hiding this comment.
as i can see, this field is unused
There was a problem hiding this comment.
it's used but passed as props, hence it's passed as a constant in useAlert return values
|
|
||
| return { | ||
| alerts, | ||
| success: (opt?: Omit<AlertOptions, 'id'>) => triggerAlert('success', opt), |
There was a problem hiding this comment.
you have incompatible types of opts, Omit<AlertOptions, 'id'> is not the same as Pick<AlertOptions, 'id' | 'icon' | 'message' | 'timeout'> used in triggerAlert
| * @param type type of alert (success, error, warning, info and default) | ||
| * @param opt alert options(timeout, message and icon) | ||
| */ | ||
| function triggerAlert(type?: Alertype, opt?: Pick<AlertOptions, 'id' | 'icon' | 'message' | 'timeout'>): void { |
There was a problem hiding this comment.
as i can see, you are using triggerAlert only inside of this composable, so why should type and opt be optional?
i suggest making of the type and opts required, also message field should not be optional in AlertOptions to avoid empty alerts
There was a problem hiding this comment.
i think passing a default message should be more helpful, something like
message: 'codex' over
message: '' (empty string)
| /** | ||
| * Various alert theme type | ||
| */ | ||
| export type AlertTheme = 'grass' | 'red' | 'amber' | 'graphite' | ''; |
There was a problem hiding this comment.
theme is accepting globally, you don't need to define it on a component level
| direction: ltr; | ||
| padding: var(--v-padding) var(--h-padding) var(--v-padding) var(--h-padding); | ||
| border-radius: var(--radius-field); | ||
| color: var(--accent--text-solid-foreground); |
There was a problem hiding this comment.
probably --base--text-solid-foreground
| background-color: var(--base--solid); | ||
| } | ||
|
|
||
| &--error { | ||
| background-color: var(--base--solid); | ||
| } | ||
|
|
||
| &--warning { | ||
| background-color: var(--base--solid); | ||
| } | ||
|
|
||
| &--info { | ||
| background-color: var(--base--solid); | ||
| } | ||
|
|
||
| &--default { | ||
| background-color: var(--base--bg-secondary); |
There was a problem hiding this comment.
you need to override color with --base--text
codex-ui/dev/Playground.vue
Outdated
| min-height: 100vh; | ||
| width: 100%; |
| background-color: var(--base--bg-primary); | ||
| color: var(--base--text); | ||
| min-height: 100%; | ||
| min-height: 100vh; |
|
|
||
| &--bottom-center { | ||
| left: 50%; | ||
| bottom: 2rem |
There was a problem hiding this comment.
| bottom: 2rem | |
| bottom: var(--spacing-l) |
| import type { AlertOptions, AlerType } from './Alert.types'; | ||
|
|
||
| /** | ||
| * |
| export const useAlert = createSharedComposable((): UseAlertComposableState => { | ||
| const alerts = ref<AlertOptions[]>([]); | ||
|
|
||
| const defaultAlertOpt = ref<AlertOptions>({ |
There was a problem hiding this comment.
redundant duplication. Default props are defined in the Alert.vue
| </div> | ||
| </div> | ||
|
|
||
| <AlertContainer /> |
There was a problem hiding this comment.
lets move it to the root component (Playground.vue) to reuse the single container for the whole app
| warning: (opt: Omit<AlertOptions, 'id'>) => triggerAlert('warning', opt), | ||
| info: (opt: Omit<AlertOptions, 'id'>) => triggerAlert('info', opt), | ||
| alert: (opt: Omit<AlertOptions, 'id'>) => triggerAlert('default', opt), | ||
| defaultAlertOpt, |
There was a problem hiding this comment.
This has been removed
| ...opt }); | ||
|
|
||
| setTimeout(() => { | ||
| alerts.value.splice(alertIndex, alerts.value.shift() as unknown as number); |
There was a problem hiding this comment.
index works unstable here since it is changing dynamically. Consider removing alert by id instead
The attached issue #247