add hamburger menu to eth page#10938
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
brad-decker
left a comment
There was a problem hiding this comment.
One minor suggestion! Great work
|
|
||
| const chainId = useSelector(getCurrentChainId); | ||
| const rpcPrefs = useSelector(getRpcPrefsForCurrentProvider); | ||
| const selectedIdentity = useSelector(getSelectedIdentity); |
There was a problem hiding this comment.
There is also getSelectedAddress selector which just returns the address. The getSelectedIdentity selector first calls getSelectedAddress then gets the full identity object by address. Might be worthwhile to use getSelectedAddress here if we don't expect to use any of the identity fields.
| import { DEFAULT_ROUTE } from '../../../helpers/constants/routes'; | ||
|
|
||
| import AssetNavigation from './asset-navigation'; | ||
| import TokenOptions from './token-options'; |
There was a problem hiding this comment.
This might be a good time to rename this to something other than "token options", since it's now being used for native assets as well.
Maybe "Asset options"? 🤔
There was a problem hiding this comment.
That makes sense to me. I'll make this change!
There was a problem hiding this comment.
@Gudahtt so I may have gone beyond what you were thinking with renaming classnames and such. There is a issue with the verify-locales process no longer seeing tokenOptions present in the UI, but I'm not sure how or if the translations should change in the locales files if we do wish to change these classnames to match the new more generic verbiage?
There was a problem hiding this comment.
Looks like E2E tests are failing from these classname changes, thought I'm not totally clear why yet
There was a problem hiding this comment.
It looks like the E2E errors are the same as the locales issues. @brad-decker @Gudahtt Should I fully revert the name changes because of the translation blocker for now? Or just revert the classnames?
b42042e to
c763c77
Compare
There was a problem hiding this comment.
You changed this translation key which is trying to find a key in app/_locales/en/messages.json named 'AssetOptions' which doesn't exist. If you do want to change this you'll need to change the key to 'assetOptions' and update 'tokenOptions' to match, as long as that's wanted.
There was a problem hiding this comment.
@brad-decker ah yes that makes more sense. I had originally changed the key in the locale files but wasn't sure that was the right thing to do given that the translations won't really match the key name anymore. Not sure what the right move is here, but feel like it could make sense to revert this key you've highlighted but leave the other name changes as is?
There was a problem hiding this comment.
In these cases we'd change en/messages.json tokenOptions: "Token options" -> assetOptions: "Asset options", then we'd make sure the code is t('assetOptions') and finally run yarn lint:fix which will remove the unused translations in the other files. At some point in the future the translation service will add the new 'assetOptions' translations to other files.
There was a problem hiding this comment.
The lack of translation here is unfortunate but not the end of the world and aligns more with our other terminology anyways. The tab is the 'assets' tab which includes the tokens that would have previously had the 'Token Options'
…ser address fetch
c763c77 to
d7c6021
Compare
(Completes) Fixes: #10148
Explanation: Adds the hamburger menu that currently exists for non-native asset token pages to the native asset (eth) page fork navigating to account details and Etherscan.
Manual testing steps: