-
Notifications
You must be signed in to change notification settings - Fork 55
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.
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
feat: click on active "joining" icon opens relevant screen #463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the activity indicator on the "Send" tab--do we even need the "Joining" indicator anymore? I almost feel like we can remove it entirely now, what do you think?
With the |
Either this, or remove the dots from the navbar links - please free me from the burden of making UI related decisions 😅
Yes, that's true! |
Let's defer the decision by dropping the "Joining"--I never liked the wording anyway. For now at least. The activity indicators do look a bit crowded up there but they're fine for now. A nice next step would be dropping those as well and replacing them with a nicely looking activity indicator (either in the header or in the footer) similar to the "Joining" thing but one that looks better and differentiates between the three different possible activities. Let's not wait around for designs though since this is a quite important PR (because of the schedule in the session response). What do you think? |
+1 |
40d3616
to
9fad9d0
Compare
Thanks for your inputs @dnlggr. Ready for re-review. |
Ah, damn, just saw this now. The way I understood it is keep the icon, but remove the "Joining" text 😬 @theborakompanioni what do you say? I think having the icon (and the functionality added in this PR that leads you to the appropriate page when you click on it) makes a ton of sense. |
Can do! It is a matter of reverting de86014 to come back to the original functionality. The "Joining" text will only appear in the mobile menu, as it would look rather weird without text. I think having both (activitiy inidicator and the icon on the right side) is fine. Will open a PR. |
Excellent, thank you! |
See #474 |
Resolves #320.
Before this commit, the "Joining" text+icon in the header nav was not clickable.
After this commit, the icon in the header nav is clickable and will either link to "Earn", "Jam" or "Send" screen.
Additional changes:
schedule
property toServiceInfo
xs
andsm
screens (side nav)These changes need JM built from the current
master
(at least including commit 6ec5c35c).Relevant commits are also part of #461 and have been cherry-picked.
📸