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

Increase File Browser Spacing #4252

Merged
merged 2 commits into from
Oct 25, 2019
Merged

Increase File Browser Spacing #4252

merged 2 commits into from
Oct 25, 2019

Conversation

WrillicR
Copy link
Member

This is the PR for #4130 . This simply increases the font-size in the file browser, thus increasing visibility and overall spacing.
Here's the before/after screenshot.
image

@tresf
Copy link
Member

tresf commented Mar 14, 2018

I really don't understand this PR...

Here's a side-by-side of 1.2.0-RC5 and Windows Explorer

image

And here's a zoomed font comparison

image

From my perspective, the font size seems sane. I'm not sure what purpose increasing it does.

@WrillicR
Copy link
Member Author

@tresf The current font size is certainly not unreadable, but increasing the font size is also for the sake of spacing. As you can see in your file explorer example, the items have an increased spacing as compared to ours, and our icons are also a bit crowded.
image
Other DAWs and systems have more spacing and a larger font size.
image
(Logic X, Ableton, LMMS (per this PR))
Instead of just increasing the padding, increasing the font size kills two birds in one stone: giving more space to each item while increasing visibility at the same time. Which, in a more practical scenario, is helpful to touch-screen users. 😃

@tresf
Copy link
Member

tresf commented Mar 14, 2018

I don't agree. If padding is needed, add padding. I'm not understanding the second bird here. I think you accidentally killed a squirrel instead. :D

@Umcaruje
Copy link
Member

I agree, if a user desires a larger text they can easily change the css but I see no justification for a larger default size

@Umcaruje
Copy link
Member

Though we sure could use some padding in there, @tresf is right

@tresf
Copy link
Member

tresf commented Mar 14, 2018

To really improve UI, and off-topic is to designate the presets with icons that distinguish them from each other.

This task is a loaded one because it brings the whole concept of recognizable, themed plugin icons into play, more toward the single-window ideas.

The font size isn't set in stone either, I just don't think stable-1.2 is in desparate need of this change. The font size seems to match that which I've grown to expect. As the software transforms, it's likely to change, but holistically.

@WrillicR WrillicR changed the title Increase File Browser Font Size Increase File Browser Spacing Mar 15, 2018
@WrillicR
Copy link
Member Author

I just don't think stable-1.2 is in desparate need of this change.

@tresf I agree, but I still like the larger font size. I've also noticed that 13px is already the browser's font size on Linux, and I definitely like the way it looks with our current theme:
image
(Images taken from Discord)
Anyways, yes, this is by no means urgent. I'd just like to point out that by switching to 13px, it becomes consistent with what's already default on Linux and removes the need for padding. I hope that I'm not beating a dead horse 😅 .

@tresf
Copy link
Member

tresf commented Mar 16, 2018

We're really starting to broach on what's best practice for sane, UI delivery -- what the OS says is sane -- vs -- what LMMS says is sane.

We may want to just ask the OS to begin with so we can split hairs, intentionally.
https://stackoverflow.com/a/40380684/3196753

The font sizing is a major problem today because some components react better than others (I'll quote #2510)

  • Green = Scales properly.
  • Red = Does not scale properly.

image

So we seem to be in agreement to match the OS, we're just not sure which OS, and we haven't done a side-by-side with MacOS yet either.

At the end of the day, I have to defer this to the theme creators and with @RebeccaDeField gone, @Umcaruje gets to decide this for the current theme.

@Wallacoloo
Copy link
Member

So @Umcaruje, is the verdict to keep the existing font size, but increase the padding? Would like something definitive so that we can move forward with this PR (merge it, adjust it, or close/defer).

@PhysSong PhysSong changed the base branch from stable-1.2 to master October 23, 2019 02:15
@PhysSong
Copy link
Member

Is it ready for merge?

@tresf
Copy link
Member

tresf commented Oct 23, 2019

I've already expressed my opinion about this change and the theme creators haven't chimed in. It's easy enough to revert.

@Spekular
Copy link
Member

An up-to-date screenshot would be good, as every one I've seen atill has the oncreased font size.

@RebeccaDeField
Copy link
Contributor

RebeccaDeField commented Oct 23, 2019

Missed this one because as you said I was not active at the time and it just got bumped to my inbox.

A screenshot of what the pr looks like in it's current state also seems like a good standard if we are deffering uix and design choices to a few busy devs and want to fastrack like we did with the plugin redesigns.

I have no problems with the simple spacing change to make it less crowded and from what I see umcaruje commented before it seems like there is an agreement to move forward with that aspect

@zonkmachine
Copy link
Contributor

Before --> After

before

@RebeccaDeField
Copy link
Contributor

Looks like a nice improvement without changing drastically enough to cause problems. Thumbs up from me.

@zonkmachine
Copy link
Contributor

Ok. Merging.

@zonkmachine zonkmachine merged commit 3745bfb into LMMS:master Oct 25, 2019
@zonkmachine
Copy link
Contributor

Oh, I didn't notice the origin and destination didn't match. It was intended for stable-1.2 but ended up in master. The merge went fine though. I'm happy to cherry-picking this into stable-1.2 .

@tresf
Copy link
Member

tresf commented Oct 25, 2019

I'm happy to cherry-picking this into stable-1.2 .

stable-1.2 is for critical bug-fix only, which this PR is not.

Reflexe pushed a commit to Reflexe/lmms that referenced this pull request Apr 4, 2020
Increase File Browser Spacing
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Increase File Browser Spacing
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.

8 participants