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

feat(frontend): Add warning to MessageBox component #3254

Merged

Conversation

AntonioVentilii-DFINITY
Copy link
Contributor

@AntonioVentilii-DFINITY AntonioVentilii-DFINITY commented Oct 31, 2024

Motivation

According to the new components library, we are going to use a single MessageBox component with a few levels of content (for example info, warning, success).
Specifically, we will use light-warning for the progress modal.

So, we first re-model the Info component to be inclusive of the other levels, and then use it.

Changes

  • Rename Info component to MessageBox.
  • Add two types of alert: info and light-warning (more will come according to usage). Defaults to info.
  • Adjust the colors to the level.
  • Use the MessageBox component in ProgressWizard and in AddTokensWarning.
  • Remove a sentence from the light warning usage in Progress Wizard.

Tests

Before

Screenshot 2024-10-31 at 11 20 14
Screenshot 2024-10-31 at 11 20 27

After

Screenshot 2024-10-31 at 11 14 15
Screenshot 2024-10-31 at 11 19 46
Screenshot 2024-10-31 at 14 28 07

@AntonioVentilii-DFINITY
Copy link
Contributor Author

AntonioVentilii-DFINITY commented Oct 31, 2024

OISY-179

Copy link
Member

@peterpeterparker peterpeterparker left a comment

Choose a reason for hiding this comment

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

LGTM, thx

Two naming feedbacks if interesting (😅).

One as comment and the other: personally I would maybe not name I think the component Alert because to me, an "alert" is a dialog, toast or snackbar.

<script lang="ts">
import IconInfo from '$lib/components/icons/lucide/IconInfo.svelte';

export let alertType: 'info' | 'light-warning' = 'info';
Copy link
Member

Choose a reason for hiding this comment

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

light-warning? not just warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

light-warning, because we have a warning level according to design, that is different

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but alertType is not about the style but about the intent no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe I shall rename the prop with componentStyle, where component is what better name we can find instead of Alert eheh

Any suggestion?

src/frontend/src/lib/components/ui/Alert.svelte Outdated Show resolved Hide resolved
@AntonioVentilii-DFINITY
Copy link
Contributor Author

RE Naming: indeed, I could not find a good name... Any suggestion? Theoretically, it should have the following levels:

plain, info, light-warning, warning, success, error

@peterpeterparker
Copy link
Member

I've got message or notification but that's meh. No better idea right now.

@AntonioVentilii-DFINITY
Copy link
Contributor Author

AntonioVentilii-DFINITY commented Oct 31, 2024

MessageBox or MessagePanel or MessageBanner? by ChatGPT

@peterpeterparker
Copy link
Member

MessageBox not that bad no?

And rename alertType to level or something?

@AntonioVentilii-DFINITY AntonioVentilii-DFINITY changed the title feat(frontend): Add warning to Alert component feat(frontend): Add warning to MessageBox component Oct 31, 2024
@AntonioVentilii-DFINITY
Copy link
Contributor Author

@peterpeterparker have a look at last commit: is level ok? or some suggestion?

Copy link
Member

@peterpeterparker peterpeterparker left a comment

Choose a reason for hiding this comment

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

Coolio for me, cool for you?

@AntonioVentilii-DFINITY
Copy link
Contributor Author

yes yes! better!

@AntonioVentilii-DFINITY AntonioVentilii-DFINITY merged commit 44dd510 into main Oct 31, 2024
11 checks passed
@AntonioVentilii-DFINITY AntonioVentilii-DFINITY deleted the feat(frontend)/Add-warning-to-Alert-component branch October 31, 2024 17:16
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.

3 participants