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.

Sign up for GitHub

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

Jump to bottom

Added currency option feature #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added currency option feature #8

wants to merge 1 commit into from

Conversation

SurajNarang
Copy link

Added currency option feature for improved user experience and clarity

@YahiaElTai
Copy link
Owner

Hey @SurajNarang Thanks so much for submitting the PR. The change is indeed very useful, few notes from me before merging.

  1. It would be great to add a screenshot of the UI change when you submit a PR with UI changes (saves the reviewer some time)
  2. I would also be great if you could keep all the JS within the js file and not in index.html
  3. The design of the currency selector is a bit off from the design of the app, It would be great to add some CSS to it to make it look nice.
  4. The currency selector I think would look better if it was somewhere around where the balance is shown
  5. Just an idea: It would be nice if the currency was displayed with the balance

Hope you can find these comments helpful and thank you for the PR :)

@YahiaElTai YahiaElTai self-requested a review December 12, 2022 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YahiaElTai YahiaElTai Awaiting requested review from YahiaElTai

Assignees

@SurajNarang SurajNarang

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SurajNarang @YahiaElTai