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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: click on active "joining" icon opens relevant screen #463

Merged
8 commits merged into from
Aug 12, 2022

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Aug 10, 2022

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:

  • Added schedule property to ServiceInfo
  • Changed "Joining" icon color from gray to green (like in the ticket screenshot)
  • Text "Joining" is only shown on xs and sm screens (side nav)
  • Added Joining indicator to "Send" nav link when single collaborative tx is active (debatable!)

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.

馃摳

@theborakompanioni theborakompanioni added the UI/UX Issue related to cosmetics, design, or user experience label Aug 10, 2022
@theborakompanioni theborakompanioni self-assigned this Aug 10, 2022
@theborakompanioni theborakompanioni requested a review from a user August 10, 2022 13:49
@theborakompanioni theborakompanioni marked this pull request as ready for review August 10, 2022 13:49
Copy link

@ghost ghost left a 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?

src/components/Navbar.jsx Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Aug 11, 2022

With the schedule in the session, we can probably also remove the forced schedule polling in the Jam screen right? If so I'll create an issue for that.

@theborakompanioni
Copy link
Collaborator Author

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?

Either this, or remove the dots from the navbar links - please free me from the burden of making UI related decisions 馃槄

With the schedule in the session, we can probably also remove the forced schedule polling in the Jam screen right? If so I'll create an issue for that.

Yes, that's true!

@ghost
Copy link

ghost commented Aug 11, 2022

Either this, or remove the dots from the navbar links - please free me from the burden of making UI related decisions 馃槄

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?

@dergigi
Copy link
Contributor

dergigi commented Aug 11, 2022

Let's defer the decision by dropping the "Joining"--I never liked the wording anyway.

+1

@ghost ghost mentioned this pull request Aug 12, 2022
src/index.css Outdated Show resolved Hide resolved
@theborakompanioni theborakompanioni force-pushed the joining-icon-link branch 2 times, most recently from 40d3616 to 9fad9d0 Compare August 12, 2022 08:57
@theborakompanioni
Copy link
Collaborator Author

Thanks for your inputs @dnlggr. Ready for re-review.

@ghost ghost merged commit 033babd into master Aug 12, 2022
@ghost ghost deleted the joining-icon-link branch August 12, 2022 09:01
@dergigi
Copy link
Contributor

dergigi commented Aug 18, 2022

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.

@theborakompanioni
Copy link
Collaborator Author

@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.

@dergigi
Copy link
Contributor

dergigi commented Aug 18, 2022

Will open a PR.

Excellent, thank you!

@theborakompanioni
Copy link
Collaborator Author

Will open a PR.

Excellent, thank you!

See #474

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI/UX Issue related to cosmetics, design, or user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header: click on active "joining" icon should open relevant screen
2 participants