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

Functionality to add on TPoS #30

Merged
merged 7 commits into from
Nov 13, 2023
Merged

Functionality to add on TPoS #30

merged 7 commits into from
Nov 13, 2023

Conversation

talvasconcelos
Copy link
Collaborator

@talvasconcelos talvasconcelos commented Nov 9, 2023

Closes #28, closes #29

@talvasconcelos talvasconcelos added the enhancement New feature or request label Nov 9, 2023
Copy link
Member

@dni dni left a comment

Choose a reason for hiding this comment

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

try to make a seperate commit for formatting next time :) as it is easier to review.
LGTM

Copy link
Collaborator

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

  1. Orange and Red colors are the other way around (DEL should be yellow since it deletes only 1 char, C should be red since it deletes all). I think we should also swap positions of DEL and C (so C is left and DEL is right). Ideally also replace "DEL" with back arrow symbol (← or ⬅).
  2. Plus functionality behaves super confusing - the OK should be always OK, the plus should be always +.
  3. There is a merge conflict now.

@prusnak
Copy link
Collaborator

prusnak commented Nov 10, 2023

I addressed my concerns above in my branch add_plus_functionality-stick. Have a look. I think it is so much less confusing that way. (Plus button just adds the current amount to the total and sets amount to 0, no other magic is happening there, no buttons are changing their functions).

My branch is still just the UI prototype to show how I think the UI should work. Submitting the form still needs fixing (it should pickup this.totalsat instead of this.sat if this.total > 0.0).

Copy link
Collaborator

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

tested ACK

@talvasconcelos
Copy link
Collaborator Author

Can we get this merged on main? I need to work on something on top of it!

@dni @motorina0 ?

@prusnak
Copy link
Collaborator

prusnak commented Nov 13, 2023

Can we get this merged on main? I need to work on something on top of it!

You have to resolve conflicts with main first (ideally with git rebase main). Then we can merge.

@talvasconcelos
Copy link
Collaborator Author

Can we get this merged on main? I need to work on something on top of it!

You have to resolve conflicts with main first (ideally with git rebase main). Then we can merge.

i don't see conflicts...

@prusnak
Copy link
Collaborator

prusnak commented Nov 13, 2023

i don't see conflicts...

Because you did not pull main?

git checkout main
git pull
git checkout add_plus_functionality
git rebase main
# ... fix conflicts
git push --force

@talvasconcelos
Copy link
Collaborator Author

talvasconcelos commented Nov 13, 2023

i don't see conflicts...

Because you did not pull main?

git checkout main
git pull
git checkout add_plus_functionality
git rebase main
# ... fix conflicts
git push --force

Nothing to rebase...

no conflicts

@prusnak
Copy link
Collaborator

prusnak commented Nov 13, 2023

Nothing to rebase...

Ah, OK. Sorry. Squashing is possible via UI, rebasing via UI is not. Let's go and merge via squash! 👍

@talvasconcelos talvasconcelos merged commit f60496e into main Nov 13, 2023
@talvasconcelos talvasconcelos deleted the add_plus_functionality branch November 13, 2023 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Idea: use different colors for POS buttons Idea: add plus (+) functionality
3 participants