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.

Sign up for GitHub

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

Jump to bottom

[ENG-3193] refactor: Refactor away from selectedOrdinal #791

Merged
merged 8 commits into from
Feb 26, 2024
Merged

[ENG-3193] refactor: Refactor away from selectedOrdinal #791

merged 8 commits into from
Feb 26, 2024

Conversation

terencehh
Copy link
Contributor

@terencehh terencehh commented Feb 5, 2024 •
edited
Loading

🔘 PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Enhancement
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

📜 Background

There is a state bug where selectedOrdinal has improper state when on multiple tabs (it overwriting the current selectedOrdinal on the page). This PR introduces a fix by removing state management for selectedOrdinal, and instead always opting to look up the ordinal using the ordinalId in URL params, and finding the selected ordinal via react-query hooks (with caching)

Issue Link: ENG-3193
Context Link (if applicable):

🔄 Changes

  • removed selectedOrdinal state actions, reducers, and setters
  • updated all selectedOrdinal usage to call react-query hook which retrieves ordinal details via the ordinalId supplied in URL params

Impact:

  • fixes state management bugs when on multiple tabs in send ordinals flow (sendOrdinal & confirmOrdinalTransaction screens)
  • indirectly improves SEO since URL contains data now

🖼 Screenshot / 📹 Video

Send/Confirm Ordinals Screen Bug video

bug.mov

Send/Confirm Ordinals Screen Fix video

fix.mov

Recover Funds/Ordinals Screen Bug video

recover.screen.bug.mov

Recover Funds/Ordinals Screen Fix video

recover.screen.fix.mov

✅ Review checklist

Please ensure the following are true before merging:

  • Code Style is consistent with the project guidelines.
  • Code is readable and well-commented.
  • No unnecessary or debugging code has been added.
  • Security considerations have been taken into account.
  • The change has been manually tested and works as expected.
  • Breaking changes and their impacts have been considered and documented.
  • Code does not introduce new technical debt or issues.

@terencehh terencehh self-assigned this Feb 5, 2024
@terencehh terencehh marked this pull request as draft February 5, 2024 14:08
@terencehh terencehh marked this pull request as ready for review February 6, 2024 02:26
jordankzf
jordankzf previously approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@jordankzf jordankzf left a comment

Choose a reason for hiding this comment

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

Well-done! Thank you.

@terencehh terencehh changed the title [ENG-3193] refactor: Refactor away from selectedOrdinal and selectedRareSat [ENG-3193] refactor: Refactor away from selectedOrdinal Feb 6, 2024
@teebszet
Copy link
Member

teebszet commented Feb 6, 2024 •
edited
Loading

nice! so much cleaner. love it when we can remove code like this

image

teebszet
teebszet previously approved these changes Feb 6, 2024
View reviewed changes
teebszet
teebszet previously approved these changes Feb 6, 2024
View reviewed changes
jordankzf
jordankzf previously approved these changes Feb 6, 2024
View reviewed changes
@terencehh terencehh mentioned this pull request Feb 7, 2024
Closed
15 tasks
@terencehh terencehh dismissed stale reviews from jordankzf and teebszet via f3d9ae7 February 7, 2024 07:12
@terencehh terencehh force-pushed the terence/eng-3193-refactor-away-from-selectedordinal-and-selectedraresat branch from f3d9ae7 to c8a5f6f Compare February 7, 2024 07:14
jordankzf
jordankzf previously approved these changes Feb 7, 2024
View reviewed changes
@DuskaT021
Copy link
Contributor

@terencehh there is a small conflict that needs to be resolved

@github-actions GitHub Actions
Copy link

Test with build here: https://github.com/secretkeylabs/xverse-web-extension/actions/runs/8018139200

@DuskaT021
Copy link
Contributor

@terencehh tested, noice, lgtm 👏

@teebszet teebszet merged commit f2072e9 into develop Feb 26, 2024
5 checks passed
@teebszet teebszet deleted the terence/eng-3193-refactor-away-from-selectedordinal-and-selectedraresat branch February 26, 2024 05:31
@github-actions github-actions bot mentioned this pull request Feb 26, 2024
Merged
teebszet added a commit that referenced this pull request Mar 14, 2024
@teebszet @terencehh
@fedeerbes @victorkirov @jordankzf
[ENG-3749] feat: runes tokens list, balances, and send flow (#80)
851598c 
* feat: init structure for send rune flow with flag for testnet only

* add hook

* add runes api client

* feat: handle rune token display on home and token dashboard (#81)

* wip

* chore: tweak token dashboard for runes and add tokens to home send bottomsheet

* remove TODOs that are handled now

* touchup

* chore: update text to support runes in receive screen

* chore: dry

* chore: remove runes list from client store

* chore: remvoe todos related to texts

* chore: remove useGetRuneFungibleTokens config todo

---------

Co-authored-by: fede erbes <[email protected]>

* Manage tokens screen for runes support (#84)

* refactor key variables to improve readability

* updates

* major refactor stx coinslist & support runes in manage token page for testnet

* remove conditonals and improve performance

* add figma for mainnet coming soon

* chore: add protocol icons for stacks, brc20 and runes tokens (#83)

* chore: add protocol icons for stacks, brc20 and runes tokens

* fix: token image loader and clean render logic

* chore: use our runes logo and remove square styles

* chore: use a default size const

* fix: font size and add missing protocol to brc20 tokens

* updates

* fix stx icon issue

---------

Co-authored-by: Terence Ng <[email protected]>

* standardize currency typing and improve variable namings

* revert logic + touchup

* touchup

* add rune icon

* add typing

* rename variable

* progress

* progress

* fix button disabled

* fix amount validations and display error msg

* chore: use core pr version with runes support

* Tim/eng 3814 receive runes screen design check (#91)

* fix: receive bottom sheet icons and blur

* Update en.json

Co-authored-by: Terence Ng <[email protected]>

---------

Co-authored-by: Terence Ng <[email protected]>

* nits

* Terence/fix edge case renderings (#95)

* fix (#94)

* fix

* Update src/app/ui-library/input.tsx

Co-authored-by: Tim Man <[email protected]>

* fix

* use variant

* use brc

* fix expression readability

* fix sip coins which are hidden by manage token list should not be sendable in send coins list (#96)

* update key

* fix: remove unused route

* fix: coin dashboard should show names and no protocol if none

* standardize ticker icon for no image tickers

* remove deprecated usage

* Update src/locales/en.json

* Update src/locales/en.json

* Update src/locales/en.json

* Update src/locales/en.json

* Update src/locales/en.json

* fix up logic

* refactor: manage tokens for runes (#98)

* refactor: manage tokens for runes

* fix: be more strict on toggle handler

* chore: minor cleanup

* chore: minor diff clean up

* update remaining FT to sip-10

* testing (#93)

* touchup currencyTypes logic to go back to using FT, fixes, prepare improved receive screen for STX, BTC, ORD

* update receive coin screen to handle STX & BTC

* fix type issue

* revert console log

* revert changes

* logic fix

* remove unused component

* Tim/refactor manage tokens (#99)

* refactor: brc20 and sip10 tokens should use react-query hooks

and no longer store balances in redux

* fix: define a redux migration callback to keep user settings for manage tokens

* fix: brc20 ft list merge

* update remaining FT to sip-10

* testing (#93)

* touchup currencyTypes logic to go back to using FT, fixes, prepare improved receive screen for STX, BTC, ORD

* update receive coin screen to handle STX & BTC

* fix type issue

* refactor: brc20 and sip10 tokens should use react-query hooks

and no longer store balances in redux

* test

* revert

* fix: also remove coins from redux state in migration

* chore: consistent naming

---------

Co-authored-by: Terence Ng <[email protected]>
Co-authored-by: Terence Ng <[email protected]>

* fix: fix typing and null case for redux migration function

* chore: bump to latest beta core

* refactorings

* typo

* use bigint and fix amount precision

* fix debounce

* update core

* remove script info

* chore: bump core version

* hack: update confirmTransaction to handle runes v1

* allow display input/output

* revert runes icon to use square

* chore: core version with undefined runes fix

* additional check

* update logo

* Empty rune list message scenario and error message (#107)

* feat: init structure for send rune flow with flag for testnet only

* add hook

* add runes api client

* feat: handle rune token display on home and token dashboard (#81)

* wip

* chore: tweak token dashboard for runes and add tokens to home send bottomsheet

* remove TODOs that are handled now

* touchup

* chore: update text to support runes in receive screen

* chore: dry

* chore: remove runes list from client store

* chore: remvoe todos related to texts

* chore: remove useGetRuneFungibleTokens config todo

---------

Co-authored-by: fede erbes <[email protected]>

* Manage tokens screen for runes support (#84)

* refactor key variables to improve readability

* updates

* major refactor stx coinslist & support runes in manage token page for testnet

* remove conditonals and improve performance

* add figma for mainnet coming soon

* chore: add protocol icons for stacks, brc20 and runes tokens (#83)

* chore: add protocol icons for stacks, brc20 and runes tokens

* fix: token image loader and clean render logic

* chore: use our runes logo and remove square styles

* chore: use a default size const

* fix: font size and add missing protocol to brc20 tokens

* updates

* fix stx icon issue

---------

Co-authored-by: Terence Ng <[email protected]>

* standardize currency typing and improve variable namings

* revert logic + touchup

* touchup

* add rune icon

* add typing

* rename variable

* progress

* progress

* chore: use core pr version with runes support

* fix button disabled

* fix amount validations and display error msg

* Tim/eng 3814 receive runes screen design check (#91)

* fix: receive bottom sheet icons and blur

* Update en.json

Co-authored-by: Terence Ng <[email protected]>

---------

Co-authored-by: Terence Ng <[email protected]>

* nits

* fix (#94)

* Terence/fix edge case renderings (#95)

* fix (#94)

* fix

* Update src/app/ui-library/input.tsx

Co-authored-by: Tim Man <[email protected]>

* fix

* use variant

* use brc

* fix expression readability

* fix sip coins which are hidden by manage token list should not be sendable in send coins list (#96)

* update key

* fix: remove unused route

* fix: coin dashboard should show names and no protocol if none

* standardize ticker icon for no image tickers

* remove deprecated usage

* Update src/locales/en.json

* Update src/locales/en.json

* Update src/locales/en.json

* Update src/locales/en.json

* Update src/locales/en.json

* fix up logic

* refactor: manage tokens for runes (#98)

* refactor: manage tokens for runes

* fix: be more strict on toggle handler

* chore: minor cleanup

* chore: minor diff clean up

* update remaining FT to sip-10

* testing (#93)

* touchup currencyTypes logic to go back to using FT, fixes, prepare improved receive screen for STX, BTC, ORD

* update receive coin screen to handle STX & BTC

* fix type issue

* revert console log

* revert changes

* logic fix

* remove unused component

* Tim/refactor manage tokens (#99)

* refactor: brc20 and sip10 tokens should use react-query hooks

and no longer store balances in redux

* fix: define a redux migration callback to keep user settings for manage tokens

* fix: brc20 ft list merge

* update remaining FT to sip-10

* testing (#93)

* touchup currencyTypes logic to go back to using FT, fixes, prepare improved receive screen for STX, BTC, ORD

* update receive coin screen to handle STX & BTC

* fix type issue

* refactor: brc20 and sip10 tokens should use react-query hooks

and no longer store balances in redux

* test

* revert

* fix: also remove coins from redux state in migration

* chore: consistent naming

---------

Co-authored-by: Terence Ng <[email protected]>
Co-authored-by: Terence Ng <[email protected]>

* fix: fix typing and null case for redux migration function

* chore: bump to latest beta core

* refactorings

* typo

* use bigint and fix amount precision

* fix debounce

* update core

* remove script info

* chore: bump core version

* hack: update confirmTransaction to handle runes v1

* allow display input/output

* revert runes icon to use square

* chore: core version with undefined runes fix

* additional check

* update logo

* push

* push

---------

Co-authored-by: Tim Man <[email protected]>
Co-authored-by: fede erbes <[email protected]>
Co-authored-by: Victor Kirov <[email protected]>

* update label

* fix: overflow on send rune amound input, when long balance (#105)

* fix: overflow on send rune amound input, when long balance

* refactor: move the complications to the input component file

* fix: set allowNegative=false to fix warnings for number format (#106)

* fix: set allowNegative=false to fix warnings for number format

* fix: styling of stx tx transaction history

* fix balance becoming exponential string for large digits

* fix large amount / decimal formatting

* touchup

* adjust gap to cater max scenario

* update display ellipsis for high digit/decimal values

* small fix

* fix: css balance token tile

* fix: cleaner css

---------

Co-authored-by: Terence Ng <[email protected]>

* fix (#109)

* [ENG-3193] refactor: Refactor away from selectedOrdinal (#791)

* commit changes

* remove selectedOrdinal for raresat screen

* update recover screens

* fix translation text

* revert non null assertion

* fix bug

* fix merge

* fix (#109)

* fixes (#110)

* fixes

* remove log

* disable rbf

* pull latest core to support rbf

* Update core and fix map key for txn history

---------

Co-authored-by: Victor Kirov <[email protected]>

* [ENG-3193] refactor: Refactor away from selectedOrdinal (#791)

* commit changes

* remove selectedOrdinal for raresat screen

* update recover screens

* fix translation text

* revert non null assertion

* fix bug

* fix merge

* chore: bump core to v11.1.1 (#806)

* chore: Bump core to 11.1.2 (#807)

* chore: bump to latest beta core

* update lock

* use mobile values

* fix render

* Solve missing name

* Fallback for SIP-10 FungibleTokens with missing name

* chore: bump core to v11.3.0

---------

Co-authored-by: Terence Ng <[email protected]>
Co-authored-by: Terence Ng <[email protected]>
Co-authored-by: fede erbes <[email protected]>
Co-authored-by: Victor Kirov <[email protected]>
Co-authored-by: jordankzf <[email protected]>
teebszet pushed a commit that referenced this pull request May 31, 2024
@aryzing
[ENG-3193] refactor: Refactor away from selectedOrdinal (#791)
05e00ce 
* commit changes

* remove selectedOrdinal for raresat screen

* update recover screens

* fix translation text

* revert non null assertion

* fix bug

* fix merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teebszet teebszet teebszet approved these changes

@jordankzf jordankzf Awaiting requested review from jordankzf

Assignees

@terencehh terencehh

Labels
ready for release Release 0.30.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@terencehh @teebszet @DuskaT021 @jordankzf