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

Skin fixes for 2.3 #3782

Merged
merged 7 commits into from
Apr 13, 2021
Merged

Skin fixes for 2.3 #3782

merged 7 commits into from
Apr 13, 2021

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Apr 11, 2021

@github-actions github-actions bot added the skins label Apr 11, 2021
@ronso0 ronso0 added this to the 2.3.0 milestone Apr 11, 2021
@ronso0 ronso0 marked this pull request as ready for review April 11, 2021 23:42
@Holzhaus
Copy link
Member

Holzhaus commented Apr 11, 2021

Thanks, the screenshot looks good. But I would look even better if the dark area around FX 1 and FX 3 had the same width. What do you think?

@ronso0
Copy link
Member Author

ronso0 commented Apr 12, 2021

sure it would.
I'd prefer to not make the expanded unit's label wider because it is the header for the controls below.
making the collapsed unit label wider which would steal width from the effectselectors (I still have the minimum skin size in mind where the entire skin is a bit tight)

@ronso0
Copy link
Member Author

ronso0 commented Apr 12, 2021

so there's no real solution for polishing this spot, unless we ignore the condensed layout

@Holzhaus
Copy link
Member

so there's no real solution for polishing this spot, unless we ignore the condensed layout

We could make mixing button a bit slimmer (currently it's wider than the PFL button). That would solve the issue and also look less crammed on small screens when the unit is collapsed.

@ronso0
Copy link
Member Author

ronso0 commented Apr 12, 2021

reason for the mixmode button being wide is that as square icon it looks too much like a Close button (D/W mode) and should resemble something like a crossfader mix curve. so I wouldn't want to change that.

we could move the Pfl button below the mixmode, so when expanding an Fx unit the entire control section is flipped from an h-layout to a v-layout.

but since the functional and usefull changes are in place now I'd rather postpone aesthetic fixes until the menubar topic is finished.

@Holzhaus
Copy link
Member

Tango looks still broken for me:

tango-issue

@ronso0
Copy link
Member Author

ronso0 commented Apr 13, 2021

in your screencast is there space below the mic section?
anyways, I'll take a look

@Holzhaus
Copy link
Member

Here's a new screencast that captures the whole window and shows all 3 skins.

Peek 2021-04-13 14-51

I also noticed that the padding in the effect selector in deere looks wrong, but that shouldn't block this PR.

@ronso0
Copy link
Member Author

ronso0 commented Apr 13, 2021

thanks for the overview!
I already fixed WEffectSelector:!editable::on in LateNight. easy to port to other skins
(until now I thought it's just Windows where the selector is visible when the list is unrolled)

@ronso0
Copy link
Member Author

ronso0 commented Apr 13, 2021

@Holzhaus if you find time, please check the effect selector again in Deere and Shade.
Also the Tango fx layout has a fixed height now.

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thanks, I had a look at all skins and they look fine now.

@Holzhaus Holzhaus merged commit 36d453e into mixxxdj:2.3 Apr 13, 2021
@ronso0 ronso0 deleted the skinfixes23 branch April 13, 2021 21:26
@NotYourAverageAl
Copy link
Contributor

@ronso0 this PR seems to have messed my window10 gui and effect names are cutoff. Though is looks fine on your screenshot

before
image

This PR
image

@ronso0
Copy link
Member Author

ronso0 commented Apr 15, 2021

okay, so there are actually people using LateNight

  • @1280px (or FHD @150%)
  • with super knobs
  • and with focus shown while the fx unit is collapsed

I wlll adjust the minimal size, of course.

@Holzhaus
Copy link
Member

@NotYourAverageAl it looks like the effect names were cutoff in the before screenshot, too?

@NotYourAverageAl
Copy link
Contributor

NotYourAverageAl commented Apr 15, 2021

@Holzhaus wrong choice of words...I meant the effect name box is now cutoff by the >

@ronso0 if you build it I'll find a way to break it :)

@ronso0
Copy link
Member Author

ronso0 commented Apr 15, 2021

@NotYourAverageAl fixed in #3788

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

Successfully merging this pull request may close these issues.

3 participants