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: Add owner icon to confirmations #1436

Merged
merged 4 commits into from
Jan 2, 2023
Merged

fix: Add owner icon to confirmations #1436

merged 4 commits into from
Jan 2, 2023

Conversation

usame-algan
Copy link
Member

@usame-algan usame-algan commented Dec 22, 2022

What it solves

Resolves #1279

How this PR fixes it

  • Displays the owner icon for transactions on the dashboard
  • Uses a lighter green for the confirmations background in the dashboard according to the design
  • Changes owner icon color in the queue to be consistent with the text color

How to test it

  1. Open a safe with a queued transaction
  2. Navigate to the dashboard
  3. Observe the owner icon next to the number of confirmations
  4. Navigate to the Queue
  5. The owner icon should have the same color as the text next to it

Screenshots

Screenshot 2022-12-22 at 14 14 08

Screenshot 2022-12-22 at 14 15 48

Screenshot 2022-12-22 at 14 15 55

@github-actions
Copy link

github-actions bot commented Dec 22, 2022

Branch preview

✅ Deploy successful!

https://confirmation_icon--webcore.review-web-core.5afe.dev

@github-actions
Copy link

github-actions bot commented Dec 22, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

component={OwnersIcon}
inheritViewBox
fontSize="small"
sx={{ color: ({ palette }) => palette.text.primary }}
Copy link
Member

Choose a reason for hiding this comment

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

Can this be inside the icon itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried but it overrides the other places where we use this icon.

In the queue the icon is either grey or black/green.
In the dashboard its always black/white.

Might be a good idea to make this more consistent but maybe there is a reason the icons + text are not green on the dashboard in dark mode. cc @liliiaorlenko

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

LGTM but tests are failing.

@francovenica
Copy link
Contributor

1 - Now that the background color is a dimmer green, it is lost when you hover the transaction. In the old version even by hovering you could still see the background greend color

Old:
image

New:
image

2 - In the design, even in dark mode the background color shoulud be a dim color of green:
Currently implemented:
image

Design:
image

@usame-algan
Copy link
Member Author

1 - Now that the background color is a dimmer green, it is lost when you hover the transaction. In the old version even by hovering you could still see the background greend color

I reverted the color changes and suggest we tackle it as a separate task after aligning with design.

@francovenica
Copy link
Contributor

Ok, so only the Owner Icon is being take in account for this ticket, and the icon looks good.
We will check colors in a different ticket then

@github-actions
Copy link

github-actions bot commented Jan 2, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@usame-algan usame-algan merged commit e89d8f4 into dev Jan 2, 2023
@usame-algan usame-algan deleted the confirmation-icon branch January 2, 2023 15:55
@github-actions github-actions bot locked and limited conversation to collaborators Jan 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard / Transaction queue: add the owners icon to the signatures element
3 participants