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 info box in activity page #3548

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

AntonioVentilii
Copy link
Collaborator

Motivation

We include an info box in the activity page to warn about third-parties on BTC transactions.

My understanding of the requirements is that we always show it, independently of having or not BTC transactions, hence there is no logic to show it or not.

Screenshot 2024-11-14 at 11 00 17
Screenshot 2024-11-14 at 11 00 43
Screenshot 2024-11-14 at 11 00 49

@AntonioVentilii AntonioVentilii requested a review from a team as a code owner November 14, 2024 10:05
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.

I did not had a look at the code yet.

  • Also no logic to close the information with the flag saved in local storage similar to what we already do?

  • It's expected that the style is different that the existing? The existing will be modified?

See screenshot.
Capture d’écran 2024-11-14 à 11 35 40

@AntonioVentilii
Copy link
Collaborator Author

AntonioVentilii commented Nov 14, 2024

Also no logic to close the information with the flag saved in local storage similar to what we already do?

yes, it is in the plans, but need to verify if it is by session or cached; this one will be provided in another PR, but not for now

It's expected that the style is different that the existing? The existing will be modified?

Correct, this is the new info box component from the Figma library. Not really sure if the others will be modified too, to be honest, but I can ask.

@@ -1,18 +1,19 @@
<script lang="ts">
import IconInfo from '$lib/components/icons/lucide/IconInfo.svelte';

export let level: 'info' | 'light-warning' | 'error' = 'info';
export let level: 'plain' | 'info' | 'light-warning' | 'error' = 'info';
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand "plain" in this context I have to say. 🤷‍♂️

Why not using "info"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on Figma, they have several levels for this message box, and they asked to be with white background (defined as plain) to have more contrast. The level info has a bluish background

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is "level === plain", what's a level "plain"?

But all good if it's use in Figma makes sense to use it here as well.

@peterpeterparker
Copy link
Member

but I can ask.

If you don't mind please do ask if the existing box design should be updated. Consistency is my passion 😉.

Thanks!

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

@AntonioVentilii
Copy link
Collaborator Author

@peterpeterparker I asked @artkorotkikh-dfinity to have a look: he will probably give a direct feedback or maybe ask @StefanBerger-DFINITY to follow up on this

@AntonioVentilii AntonioVentilii enabled auto-merge (squash) November 14, 2024 13:49
@AntonioVentilii AntonioVentilii merged commit cc45837 into main Nov 14, 2024
15 checks passed
@AntonioVentilii AntonioVentilii deleted the feat(frontend)/add-info-box-in-activity-page branch November 14, 2024 13:53
@artkorotkikh-dfinity
Copy link
Collaborator

I'm late! But:

  • It's expected to have a new style - we'll use these info boxes in the future as well
  • The idea was actually to have it closable. I think we can apply the same logic as in the existing Info boxes!
image

@AntonioVentilii
Copy link
Collaborator Author

AntonioVentilii commented Nov 16, 2024

OISY-130

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