fix(frontend): load Carousel when missing settings in user profile #3893
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AntonioVentilii
requested review from
peterpeterparker and
DenysKarmazynDFINITY
peterpeterparker
approved these changes
Dec 7, 2024
There was a problem hiding this comment.
LGTM, thx
Note: I actually read the code for quite some minutes because I'm not entirely sure making a distinction undefined or null should be something that happens at the derive level. Another option I guess would have been having a dedicated $userSettingsLoaded: Readable<boolean> information but just idea so all good.
…tings-in-user-profile
AntonioVentilii
deleted the
fix(frontend)/load-Carousel-when-missing-settings-in-user-profile
branch
|
@peterpeterparker makes sense! Have a look at please, is that what you thought: |
AntonioVentilii
added a commit
that referenced
this pull request
Dec 9, 2024
# Motivation According to @peterpeterparker 's suggestion in #3893 (review) , we find a better way to distinguish if the user settings are not available because the user profile is not loaded, or if they are missing (old accounts for example). The base difference is simply if the user profile is not `nullish`: if it is not `nullish` and the settings are nullish, we are sure that the `settings` are missing. # Changes - Create derived store `userProfileLoaded`. - Use new store in `DappsCarousel`. # Tests Test created and adjusted.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The carousel was not being loaded for old accounts, because
userSettingswas alwaysundefined.However, we should distinguish between not having loaded the user profile (from which
userSettingsderives), and having a missing fieldsettingsin the user profile (it happens for old accounts, since we introduced the field recently).Changes
userSettingsderived store to setnullwhen the user profile exists but it is missing thesettingsfield.DapsCarousel.Tests
Provided.