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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: show active offers #461

Merged
merged 11 commits into from
Aug 24, 2022
Merged

feat: show active offers #461

merged 11 commits into from
Aug 24, 2022

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Aug 9, 2022

Resolves #399. Also partly addresses #413 (but will be additionally tackled in a separate PR).

Shows the current active offers on the Earn page.

Please review and merge #456 before this PR.
These changes need JM built from the current master (at least including commit 6ec5c35c).

Questions:

  • 'Nickname', 'Nick' or 'Nym'?
  • Should the Nym be hidden if showBalance is disabled?
  • Show some kind of loader till the data is available? Currently it "just appears" whenever the /session response arrives..

@theborakompanioni theborakompanioni added enhancement New feature or request UI/UX Issue related to cosmetics, design, or user experience labels Aug 9, 2022
@theborakompanioni theborakompanioni self-assigned this Aug 9, 2022
@theborakompanioni theborakompanioni force-pushed the show-offer branch 2 times, most recently from 0956a19 to 9725b61 Compare August 9, 2022 13:15
@theborakompanioni theborakompanioni requested a review from a user August 9, 2022 13:19
@theborakompanioni theborakompanioni marked this pull request as ready for review August 9, 2022 13:19
@ghost
Copy link

ghost commented Aug 11, 2022

This is great! Will test it later--I'm having issues building the regtest right now. 😅

Regarding the questions:

'Nickname', 'Nick' or 'Nym'?

Honestly, I'd go for "Offer ID"--that's what it is in this context, right? I know this is the nickname on the IRC servers as well, but that's something the average user doesn't care about I guess. See also #413 where it's referred to as ID.

Should the Nym be hidden if showBalance is disabled?

Good question! How about renaming the "show/hide balance" toggle to "show/hide sensitive data" and then also hiding the nick?

Show some kind of loader till the data is available? Currently it "just appears" whenever the /session response arrives..

Without having tried it yet, what about showing the normal placeholder in the case where we can anticipate that an offer will be present in the /session response--that is when "Earn" is active?

src/i18n/locales/en/translation.json Outdated Show resolved Hide resolved
src/i18n/locales/en/translation.json Outdated Show resolved Hide resolved
src/components/Earn.jsx Outdated Show resolved Hide resolved
src/components/Earn.jsx Outdated Show resolved Hide resolved
src/components/Earn.jsx Outdated Show resolved Hide resolved
Comment on lines +50 to +75
.offerContainer {
border: 1px solid var(--bs-gray-200);
border-radius: 0.3rem;
padding: 1.25rem;
margin-bottom: 1.5rem;
}

:root[data-theme='dark'] .offerContainer {
border-color: var(--bs-gray-700);
}

.offerContainer .offerTitle {
width: 100%;
font-size: 1.2rem;
color: var(--bs-body-color);
}

.offerContainer .offerLabel {
color: var(--bs-gray-600);
font-size: 0.8rem;
}

.offerContainer .offerContent {
font-size: 0.8rem;
word-break: break-all;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the same styles as the existing FB box right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@dergigi
Copy link
Contributor

dergigi commented Aug 18, 2022

Found a small edge case, I think.

When the string contains an x between two numbers, it will be rendered as a math symbol (which is a bootstrap thing, I believe).

Screenshot 2022-08-18 at 13-24-22 Jam for JoinMarket

Might not be a big problem, because it's still an x afaik, but it looks a bit confusing (at least to me).

Here is how it looks if we wrap the whole string in a <code> tag:
Screenshot 2022-08-18 at 13-23-32 Jam for JoinMarket

@theborakompanioni
Copy link
Collaborator Author

Found a small edge case, I think.

When the string contains an x between two numbers, it will be rendered as a math symbol (which is a bootstrap thing, I believe).

Nice one! Adding css class slashed-zeroes (which is used throughout the app) will apply font-feature-settings: 'calt' off which fixes the problem. Again.. really good catch! Fixed.

@dergigi dergigi merged commit c355d41 into master Aug 24, 2022
@dergigi dergigi deleted the show-offer branch August 24, 2022 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request UI/UX Issue related to cosmetics, design, or user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Earn: show offer and fee settings when earn is active
2 participants