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

Tango: style Quick Effect selector #4503

Merged
merged 5 commits into from
Dec 11, 2021
Merged

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Nov 6, 2021

based on #4501

updated:
image

@ronso0 ronso0 added this to the 2.4.0 milestone Nov 6, 2021
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Please rebase this to main. Without a rebase we see rebased commits from the already merged PRs twice in the history.

@daschuer
Copy link
Member

daschuer commented Nov 7, 2021

The rest looks good.

@ronso0
Copy link
Member Author

ronso0 commented Nov 7, 2021

Please rebase this to main. Without a rebase we see rebased commits from the already merged PRs twice in the history.

That's how I planned to do it.
I wonder why the log doesn't have duplicate commits after merging the LateNight fx PR.

Copy link
Contributor

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

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

The removal of the border and making the background identical to the surroundings makes it hard to tell that the comboboxes are comboboxes. This PR is a step backwards IMO. Can you explain why you think these changes are an improvement?

@daschuer
Copy link
Member

daschuer commented Nov 7, 2021

I wonder why the log doesn't have duplicate commits after merging the LateNight fx PR.

These commits where identical, inclusive their history. Here they are almost identical, with a different commit hash.

Comment on lines -1737 to +1739
WEffectChainPresetSelector {
background-color: #252525;
color: #888;
WEffectChainPresetSelector {
/* blend with the mixer backgrpund */
background-color: #333;
color: #aaa;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this.

@Swiftb0y
Copy link
Member

Swiftb0y commented Nov 7, 2021

IMO its still visible that the effect name is a clickable combobox because of the "down-arrowhead" next to the name.
Removing the border to remove visual clutter makes sense here.

@Holzhaus
Copy link
Member

Holzhaus commented Nov 7, 2021

The removal of the border and making the background identical to the surroundings makes it hard to tell that the comboboxes are comboboxes. This PR is a step backwards IMO. Can you explain why you think these changes are an improvement?

I think it mainly saves screen space that would otherwise be used by margins/padding/borders. I'm also torn on this, because I think it space-efficient and looks good, but on the other hand it's less easy to discover. Maybe we can consider adding borders and reducing font size instead.

@Swiftb0y
Copy link
Member

Swiftb0y commented Nov 7, 2021

If we talk about discoverability, I think thats fine too. It's not an essential feature that you need to see understand and instantly (like the playbutton for example). Its not like its deeply buried in menus (unless we hide it using a skin or preference option, which is not good idea IMO).

@Be-ing
Copy link
Contributor

Be-ing commented Nov 7, 2021

I agree that it isn't crucial that this is super easy to discover and even with this change it isn't too hard to discover, but still, this makes it harder to discover and I don't think there was a problem with the styling before.

@Swiftb0y
Copy link
Member

Swiftb0y commented Nov 7, 2021

I guess its a personal preference call, and I prefer this. Should we decide via a quick poll?

@Be-ing
Copy link
Contributor

Be-ing commented Nov 7, 2021

I would like to hear @ronso0's motivation for this change before jumping to a vote.

@ronso0
Copy link
Member Author

ronso0 commented Nov 7, 2021

My motivation for this plain effect selector

  • with borders it's the inner padding + border + margin to the EQs and bottom container border that would make the entire mixer 'much' taller and the decks would grow unnecessarily -- especially in Tango which is meant to be as compact as possible
  • it has the same drop-down arrow indicating a combobox as the effect selector, the AutoDJ mode selector and the search box, as well as a hover effect, thus I consider it discoverable
  • I do not expect the majority of users to change this frequently, if at all, thus styling it more like an informative label makes much more sense than adding the visual clutter of a fully decorated combobox

Copy link
Contributor

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

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

There is less room for text with this branch. Here is main:
image

This branch:
image

@ronso0
Copy link
Member Author

ronso0 commented Nov 14, 2021

The previous implementation was quick and dirty and always added extra row to the deck.
It was okay for testing but I'm not going to premanently change the layout that much just to have a large quick effect selector.

Though, I can try to squeeze out a few more pixels for the selector.

@ronso0
Copy link
Member Author

ronso0 commented Nov 20, 2021

image

Note: the screenshot was made with #4532

@ronso0 ronso0 marked this pull request as draft November 20, 2021 00:20
@ronso0
Copy link
Member Author

ronso0 commented Nov 20, 2021

marked as draft because it looks like I broke the fx units. I'll remove the extra tweaks and add the chain preset button styling asap.

@Be-ing
Copy link
Contributor

Be-ing commented Nov 20, 2021

I wouldn't be opposed to that last screenshot, though I still prefer how it is currently in the main branch.

@ronso0
Copy link
Member Author

ronso0 commented Nov 30, 2021

image

Ready.

@ronso0 ronso0 marked this pull request as ready for review November 30, 2021 00:41
@ronso0
Copy link
Member Author

ronso0 commented Nov 30, 2021

In another PR I'll make the fx unit menu more discoverable (WEffectChainPresetButton), the arrow was just a quick shot, too. I used the gear icon (skin settings) for that in LateNight and it makes more sense than the down arrow.

@Swiftb0y
Copy link
Member

Swiftb0y commented Dec 1, 2021

Why did you put the PFL button the same height as the dropdown? Wouldn't it make more sense to make the vumeter shorter to put the pfl button horizontally aligned with the filter and gain knob so there is more space for the text in dropdown?

@ronso0
Copy link
Member Author

ronso0 commented Dec 2, 2021

Yes, that would look better for that one configuration.
But it has some consequences and requires more elaborate changes to keep the skin consistent (trying to keep my initial design intact).

  • channel VUs are aligned with the main VU (shrink the main VU, too?)
  • with EQ knobs hidden the Vol slider and VU should expand to available height. The VU pixmap are designed pixel-perfect so we'd need two VU meters per deck? Or have one version center vertically?

Tbh I'm not motivated to work on that right now. The Deere implementation is still overdue, LateNight still needs the chain menu button...

@ronso0
Copy link
Member Author

ronso0 commented Dec 2, 2021

That said, I'd like to merge this now and me or anyone else may propose improvements later on.

@Holzhaus Holzhaus requested a review from Be-ing December 3, 2021 00:09
@Swiftb0y
Copy link
Member

Swiftb0y commented Dec 3, 2021

Thanks for the explanation. Sure we can merge.

@ronso0
Copy link
Member Author

ronso0 commented Dec 3, 2021

@Be-ing already agreed to the new implementationearlier so I don't think we need to wait for his formal approval.

@Swiftb0y
Copy link
Member

Swiftb0y commented Dec 3, 2021

He also recently announced to abstain from contributing to mixxx for the foreseeable future, I don't think he'll care to review/approve this PR because of that.

@ronso0
Copy link
Member Author

ronso0 commented Dec 10, 2021

pa-ping...

@Holzhaus
Copy link
Member

Alright, shall we merge then?

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thanks 👍

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

Successfully merging this pull request may close these issues.

5 participants