Fix collectible hover button #4971
Comments
fabric-8
added
bug-p3
markmhendrickson
added
needs:visual-improvement
and removed
bug-p3
|
I made this change as requested @fabric-8 . Let me know if it looks OK Area.mp4 |
|
@pete-watters Thanks for improving this! Minor thing I am wondering about: It's still using the rounded shape / prior size, any chance we can align this with the design system so it uses a 32x32 rectangular shape with border-radius? |
Sure. I hadn’t noticed that, I will standardise it with the design system ASAP |
|
Does this look OK now? I don't have a good eye for these small changes New: Old: |
## [6.42.2](v6.42.1...v6.42.2) (2024-07-02) ### Bug Fixes * add ledger test for bitcoin api calls, close leather-io/issues[#114](#114) and [#5574](#5574) ([26b6b4f](26b6b4f)) * add network form bitcoin network selector ([2169065](2169065)) * adjust collectible arrow hover transparancy, closes [#4971](#4971) ([4571ad7](4571ad7)) * apply spamFilter to all crypto captions and titles, ref leather-wallet/issues[#5589](#5589) ([ed284b9](ed284b9)) * asset row skeleton loader, closes [#5466](#5466) ([9b1c8c7](9b1c8c7)) * integrate tabs, toast, avatar, address ref leather-wallet/issues[#64](#64) ([930c125](930c125)) * ledger bitcoin query ([a11f6a6](a11f6a6)) * refactor dialog to use IconButton to fix title without affecting header, closes [#5470](#5470) ([a5011ec](a5011ec)) * remove extra div, ref leather-io/issues[#5469](#5469) ([abdaab5](abdaab5)) * responsive width for popup ([c61ced0](c61ced0)) * roll back import of dropdown menu, ref leather-wallet/issues[#64](#64) ([b03d6af](b03d6af)) * stx send form high fee warning, closes [#5362](#5362) ([0af3ae9](0af3ae9)) * **ui:** center header, ref leather-io/issues[#5470](#5470) ([53d1e6c](53d1e6c)) * use dialog from monorepo, ref leather-wallet/issues[#108](#108) ([de2b5a0](de2b5a0)) * use DropDown menu from monorepo for Settings, ref leather-wallet/issues[#102](#102) ([65131b0](65131b0)) ### Internal * **analytics:** use uniform api with middlewear, remove wrapper fns ([c82ed26](c82ed26)) * install @leather-wallet/ui package, ref leather-wallet/issues[#64](#64) ([12dca36](12dca36)) * post-release merge back ([834d399](834d399)) * remove radix-ui base css styles, ref leather-wallet/issues[#104](#104) ([d5ebb6b](d5ebb6b)) * rename github org ([5d42eeb](5d42eeb)) * rename npm package scope ([7851c44](7851c44)) * specify resolution for ws ([ff74653](ff74653)) * undo width change ([d3595f4](d3595f4)) * update dependancies ([62500b8](62500b8)) * update monorepo dependancies ([7ec537b](7ec537b)) * update monorepo packages ([263adcd](263adcd)) * upgrade react query to v5 ([6184a3d](6184a3d))
|
@pete-watters are you referring to this design guidance in the description of this issue? It looks quite different than your latest change, in terms of icon asset and styles 
|
Let's pull the details directly from Figma so they don't have to be eye-balled? |
|
@pete-watters All that needs to be done here:
|
|
Thanks @markmhendrickson & @fabric-8 . I was working on this yesterday evening but forgot to update the ticket. My mistake here was I focused on fixing the hover effect and didn't actually realise I had to replace the icon also. (@fbwoolf helped me spot that) I have it working as you say above Fab but when I check Figma this button has a padding of No button padding:
Padding:
I take it that this padding is necessary so it looks right? When I add padding it is effecting the hover state and there is a small gap around the edges. I can continue to work on that |
|
Yes with padding looks good and sorry for the hassle 🙈 |
No need to be sorry, it's my fault, I should have checked the screenshot in more detail before. I'll open a PR for review with the padding as it doesn't look 100% right. It's hard to see in the screenshot but there is a slight visible gap in the hover overlay due to the padding |
|
@fbwoolf said she can add the final touches here and get the hover effect working in the Thanks Fara! |
|
@pete-watters the fix ended up being real simple here based on my changes in this commit: All I had to do was add a
|
Thanks @fbwoolf 👏 I tested it and it's looking great. I'm not sure how I didn't figure this out but glad to be part of a great team |
## [6.43.0](v6.42.2...v6.43.0) (2024-07-15) ### Features * add Leather to WBIP004 array, closes [#5615](#5615) ([e38f6ab](e38f6ab)) * mock mainnet btc blockstream requests ([16d751c](16d751c)) ### Bug Fixes * choose account prevent bug, closes [#5509](#5509) ([89500a8](89500a8)) * collectible hover, closes [#4971](#4971) ([7728eeb](7728eeb)) * confine clickable area of account switcher to account name and chevron, closes [#5621](#5621) ([472c7e4](472c7e4)) * dust change amounts, closes [#4979](#4979) ([8b40ea7](8b40ea7)) * increase zIndex of tooltip to prevent it being obscured, closes [#5622](#5622) ([a1f86bb](a1f86bb)) * show account name in signPsbt and signBip322 header, ref [#4657](#4657) [#4859](#4859) ([71f2565](71f2565)) ### Internal * add new analytics events ([3f9548e](3f9548e)) * post-release merge back ([c1bbf89](c1bbf89)) * reenable swaps, closes leather-io/issues[#98](#98) ([5faba22](5faba22))
When hovering the callout button that appears on top of the collectible preview, the button becomes transparent. Instead we'd like the button to stay opaque while applying the hover effect as an overlay.
Solution suggestion: At this point this is a one-off case for collectibles. Hence we suggest to solve this locally vs. systematically. For example, we could simply wrap the button in a container with bg background-primary, with the button staying transparent and using the transparent hover effect via the corresponding component background hover token (as is applied now).
PS: New icon is assuming new icon library is in place
The text was updated successfully, but these errors were encountered: