[DDW-1111] Non-recommended decimal place setting alert is not displayed before 1st change of settings #3007
Conversation
marcin-mazurek
changed the title
…ecommended-decimal-settings-warning-not-shown
|
|
||
| export type WalletLocalData = { |
There was a problem hiding this comment.
Moved those to separate file.
| decimals: hasSavedDecimals | ||
| ? savedDecimals | ||
| : recommendedDecimals || DEFAULT_DECIMAL_PRECISION, | ||
| decimals: hasRecommendedDecimals |
There was a problem hiding this comment.
The correct logic is: when the user opens the settings, if the token has recomended decimal, it should use it as the initial value, so it can be saved right away (in fact - fixed). If there is no recommendation we should show the current or fallback value.
There was a problem hiding this comment.
What if the user already has saved decimals? Would it display the recommended anyway?
There was a problem hiding this comment.
That is my understanding of the requirement. @dmitrii-gaico could you confirm?
There was a problem hiding this comment.
Yeah, that also seems a bit weird to me 🤔 I would also think that the saved decimals always trump the recommended ones (if there was anything saved ofc) like it was before
There was a problem hiding this comment.
Well, so I personally believe we should never change the initial form value, we should always show the current setting no matter what is recommended. As a user, when I open the edit form, I expect to see the currently applied values, no matter if these are defaults or set by me explicitly. Unless that value can no longer be used (eg. because the value selected is no longer available).
Initially when I tried to understand what was the problem, I really couldn't wrap my head around the UX of this form until I jumped on a call with @dmitrii-gaico and he explained it to me.
I would personally show a red screaming alert that the current setting is different from recommended setting (instead of tiny warning sign with no text) and give the user the chance to decide. Or at least add one more section that shows the current setting, because at the moment the user cannot clearly see what is the currently used decimal (or maybe I still don't understand this feature).
But - this is my personal opionion - and I guess this was already discussed in the past and carefully thought through.
There was a problem hiding this comment.
To be fair, as a user of multiple wallets, this is something unique to Daedalus and I was also initially confused in the beginning. But in my experience, initial state is initial (default) state unless there has been a write operation where that bit of state has been overridden.
There was a problem hiding this comment.
As discussed with @dmitrii-gaico, this change should be reverted to the original implementation.
| TransportDevice, | ||
| } from '../../../common/types/hardware-wallets.types'; | ||
|
|
||
| export type WalletLocalData = { |
There was a problem hiding this comment.
Moved those from source/renderer/app/api/utils/localStorage.ts
iohk-bors
bot
deleted the
fix/ddw-1111-non-recommended-decimal-settings-warning-not-shown
branch
danielmain
added
release-5.0.0
This PR fixes the issue with non-recommended decimal place setting not displayed before the first change of settings.
Related PR: #2905
Screenshots
No UI changes.
Testing Checklist
Review Checklist
Basics
input-output-hk/daedalus-devandinput-output-hk/daedalus-qaassigned as PR reviewersrun Chromaticlabel to PR to trigger the run)release-vNext,feature/bug/chore,WIP)yarn manage:translationsproduces no changes)yarn storybook)yarn.lockfile is updatedCode Quality
Testing
After Review