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

Update UI of Temperament Widget #2776

Merged
merged 16 commits into from
Feb 4, 2021

Conversation

daksh4469
Copy link
Member

@daksh4469 daksh4469 commented Jan 27, 2021

Minor UI Improvement: Related to #2767

This PR resolves a UI inconsistency in the temperament widget in which several buttons would have the cursor as "pointer" on mouseover while others did not. Also, the text in various buttons was not vertically central-aligned. Specifically, once clicked on the "Preview" button in the "Equal" and "Ratios" tab, the appearing "Back" and "Done" buttons had this issue:

image

Also, the "Clear" button in this widget did not have a pointer cursor. Finally, all button-texts were vertically central-aligned.
After resolving these, final result looks like this:

image
image

Copy link
Member

@ricknjacky ricknjacky left a comment

Choose a reason for hiding this comment

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

LGTM.

Concatenate your commits, it's a small change. Practice segregating your chances into small commits when the modifications are huge.

Also, once the PR that ports temperament widget is merged, you should update your file accordingly, rebase then push else there might be Merge Conflicts.

@meganindya
Copy link
Member

Why does the color look different?
Anyway please fix the conflict.

@daksh4469
Copy link
Member Author

The color looks different due to oversaturation which is an error in Ubuntu on my laptop. No worries about that.
Also, I will resolve the conflicts ASAP.

@meganindya
Copy link
Member

Please don't merge master into PR branches. Creates more trouble. Always rebase.

@daksh4469
Copy link
Member Author

I will surely keep this in mind from now on. How can I fix this now?

@ricknjacky
Copy link
Member

How can I fix this now?

As I mentioned in my comment above, update your local, rebase,then push

If you're looking for Steps:-

Switch to master branch

Set upstream
git remote add upstream {repo's link}
git fetch upstream
git merge upstream/master
git push origin master

then rebase your commits into one, you can use
git rebase -i head~x

Then eventually git push -f origin <this PR's branch name>

Hope this helps, if you have any queries please do ask.

@meganindya
Copy link
Member

I already did the rebase in this case.

@ricknjacky
Copy link
Member

I already did the rebase in this case.

Yeah I just saw. My bad missed it previously.

@daksh4469
Copy link
Member Author

Thanks, @ricknjacky for the steps. I didn't know about rebase earlier.

Copy link
Member

@meganindya meganindya left a comment

Choose a reason for hiding this comment

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

While you're at it ... how about you try a few more cosmetic changes on this widget?

Screenshot (98)
Notice the size, and the position of the play buttons

Screenshot (99)
These circles could have pointer too

Screenshot (100)
The backgrounds can be improved

@daksh4469
Copy link
Member Author

I was also thinking of some ideas related to the UI of this widget. My take:

  • Add some padding to the "Octave Space" tab.
  • Align the play buttons with their respective rows.
  • Add pointer to circles (as mentioned by @meganindya).
  • Maybe add a shade of RED background color to the "Close" button.

@meganindya
Copy link
Member

Defer any cosmetic changes to widgetWindow for now. Rest works.

@meganindya meganindya marked this pull request as draft January 28, 2021 06:47
@meganindya
Copy link
Member

Clear draft when finally ready for review. You could comment to showcase intermediate progress though.

@daksh4469
Copy link
Member Author

Can you please guide me through the process to update my local repo?
I tried running:

git pull upstream master

But it showed merge conflicts.

image

And then when I resolved these conflicts, it showed many errors in the code of temperament.js file

@meganindya
Copy link
Member

meganindya commented Jan 28, 2021

First reset on your local machine to the commit SHA where you created the branch. Then do a git pull origin daksh4469-master. I've rebased the branch on top of master again.

@meganindya
Copy link
Member

I don't see any conflicts anymore.

b19ded3 should be the commit SHA of your branch's HEAD.

@daksh4469
Copy link
Member Author

daksh4469 commented Jan 28, 2021

I just updated my local repo. Thanks, @meganindya for the instructions.
However, I noticed a bug in this updated code in which the tab "C Major" between the "Note" and "Frequency" tabs is now shown as "undefined undefined".

image

@meganindya Can you please verify this?

@meganindya
Copy link
Member

Works fine on mine.

e04a9c45-4b22-4365-a8a6-b4f9aa35289e

@daksh4469
Copy link
Member Author

Hey @meganindya....
Just an update. I made the following changes in the Arbitrary Tab in the Temperament widget. Can you please verify this so I can commit these changes and work further.

Before Changes:

image

After Changes:

Screenshot from 2021-02-02 21-40-35

Also, I have changed the background colors of other tabs to White as shown below:

Before Changes:

image

After Changes:

image

@meganindya
Copy link
Member

Yeah OK.

@daksh4469 daksh4469 changed the title Update Button styling in Temperament widget. Update UI of Temperament Widget Feb 3, 2021
Copy link
Member

@meganindya meganindya left a comment

Choose a reason for hiding this comment

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

sns
Not a good practice to merge master branch commits into feature branches. Might create lots of unnecessary problems.

@meganindya
Copy link
Member

Let me fix the branch.

@meganindya
Copy link
Member

BTW, did you create a feature branch, or you pushed directly onto your master and creating the PR created the feature branch daksh4469-master?

@meganindya
Copy link
Member

Branch updated. On your local machine, do a git checkout daksh4469-master followed by git reset --hard 3b1f058b4667314da7586748d88cd88086966e48

@daksh4469
Copy link
Member Author

daksh4469 commented Feb 3, 2021

I created the branch daksh4469-master and have been working on that branch only since beginning. I haven't done anything on my master branch.
Am I doing something wrong in this?

@meganindya
Copy link
Member

Added an improvement. If you're working on something then rebase to bd95746, else reset to bd95746

@meganindya
Copy link
Member

Mark this PR as ready for review when you're done. Everything else looks fine.

@daksh4469
Copy link
Member Author

I ran the command git reset --hard bd95746b1f27b7a4eba520c0dc3a07b4378ca885....but got the following error:

image

@meganindya
Copy link
Member

Reset to the base commit of this PR then do a git pull.

@daksh4469 daksh4469 marked this pull request as ready for review February 3, 2021 19:17
@meganindya meganindya merged commit 55051a7 into sugarlabs:master Feb 4, 2021
@daksh4469
Copy link
Member Author

Hey @meganindya,
Should I now delete the branch daksh4469-master and make a new one next time? Or update it with recent commits?

@meganindya
Copy link
Member

delete

@daksh4469 daksh4469 deleted the daksh4469-master branch February 4, 2021 06:22
@daksh4469
Copy link
Member Author

Okay,
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants