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.

Sign up for GitHub

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

Jump to bottom

Miscellaneous improvements to settings page styling #4235

Merged
merged 4 commits into from
Nov 28, 2023
Merged

Miscellaneous improvements to settings page styling #4235

merged 4 commits into from
Nov 28, 2023

Conversation

MitchelPaulin
Copy link
Contributor

@MitchelPaulin MitchelPaulin commented Oct 26, 2023 •
edited
Loading

Miscellaneous improvements to settings page styling

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Some styling and organization improvements I noticed while working on a different issue

Related issue

N/A

Description

I've noticed some inconsistencies when going through the settings, or some things I think can be organized better. I've tried to capture the low hanging fruit here. I've enumerated all changes in the screenshots.

Screenshots

Organize Data Settings By Type

Just a more clean layout I think.

Before After
image image

Fix Search Spacing

The search already had a bottom 10px border, added a top one as well, which makes labels appear less crowded.

Before After
image image

Testing

Only visual changes were made

Desktop

  • OS: Pop_OS
  • OS Version: 22.04 LTS
  • FreeTube version: 0.19.1

Additional context

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 26, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 26, 2023 03:31
auto-merge was automatically disabled October 26, 2023 03:55

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 26, 2023 03:55
efb4f5ff-1298-471a-8973-3d47447115dc
efb4f5ff-1298-471a-8973-3d47447115dc reviewed Nov 22, 2023
View reviewed changes
Copy link
Member

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We want to do this with the coloring of the buttons. if u want to include it in this PR that would be nice but i understand that If u think that is too much to incorporate in this PR because it supposed to be small styling changes u can remove that code.

The other changes lgtm though

auto-merge was automatically disabled November 22, 2023 22:50

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 22, 2023 22:50
@MitchelPaulin
Copy link
Contributor Author

We want to do this with the coloring of the buttons. if u want to include it in this PR that would be nice but i understand that If u think that is too much to incorporate in this PR because it supposed to be small styling changes u can remove that code.

The other changes lgtm though

I think its best to have that in a different PR, I'll remove the button changes for now

@kommunarr
Copy link
Collaborator

kommunarr commented Nov 22, 2023 •
edited
Loading

If you're interested in solving this, one settings gripe I have is that the different rows aren't left-aligned properly for either column in Distraction Free Settings.

Screenshot_20231122_170851

@shadycloud
Copy link

If you're interested in solving this, one settings gripe I have is that the different rows aren't left-aligned properly for either column in Distraction Free Settings.

Agreed.

@absidue
Copy link
Member

absidue commented Nov 23, 2023

@jasonhenriquez the columns aren't meant to be left aligned in the existing code, the columns are spread evenly, and each group is independent. Not sure how you would write CSS that can do the same, across multiple independent groups.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

Shamelessly going to put one gripe here i have. That bottom slider not being aligned with the top ones
Capture1

@absidue
Copy link
Member

absidue commented Nov 23, 2023

Sounds like that is going to be a bunch of development and testing, to figure out how to address that in a way that works for all settings sections and keeps the dynamic layout that adapts to different screen widths. Therefore I suggest that it be left to a later pull request.

efb4f5ff-1298-471a-8973-3d47447115dc
efb4f5ff-1298-471a-8973-3d47447115dc previously approved these changes Nov 23, 2023
View reviewed changes
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@kommunarr
Copy link
Collaborator

@jasonhenriquez the columns aren't meant to be left aligned in the existing code, the columns are spread evenly, and each group is independent. Not sure how you would write CSS that can do the same, across multiple independent groups.

One way would be using display: grid to create a repeating pattern. Or getting rid of the switchColumnGrid wrappers and using display: flex, with titles having a full-width flex-basis and switches having one of 50%, using some combination of max-content and alignment properties. Totally fine if this is skipped on if it's non-trivial, but it should be doable with some slight tinkering to the template.

@MitchelPaulin
Copy link
Contributor Author

If you could throw these other styling problems into an issue I wouldn't mind taking a look at them, but I'd like to just merge this PR as is then go from there.

kommunarr
kommunarr reviewed Nov 23, 2023
View reviewed changes
Copy link
Collaborator

@kommunarr kommunarr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

suggestion (non-blocking): for Data Settings, use justify-content: center and reduce heading margin-block to 0.5em for a cleaner look.

Screenshot_20231123_102844

@absidue
Copy link
Member

absidue commented Nov 23, 2023 •
edited
Loading

This pull request copies the groupTitle class from the distraction free settings template, but didn't copy over the CSS for it, that centred the title. I suggest moving it from the distraction free css file into the ft-settings-section scss file (remember to add :deep), so that it applies to both sections and then adding @jasonhenriquez's margin-block suggestion there.

@MitchelPaulin
Copy link
Contributor Author

image

Added the suggestions

auto-merge was automatically disabled November 27, 2023 23:12

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 27, 2023 23:13
@FreeTubeBot FreeTubeBot merged commit 245fb12 into FreeTubeApp:development Nov 28, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 28, 2023
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Nov 29, 2023
@PikachuEXE
Merge branch 'development' into feature/playlist-2023-05
1feec0b 
* development: (38 commits)
  Channel subscribe button in search results (FreeTubeApp#4376)
  Translated using Weblate (Czech)
  Translated using Weblate (Portuguese (Brazil))
  Use F5 to refresh subscriptions (FreeTubeApp#4399)
  Miscellaneous improvements to settings page styling (FreeTubeApp#4235)
  Translated using Weblate (Icelandic)
  Translated using Weblate (Swedish)
  Translated using Weblate (Arabic)
  Bump lefthook from 1.5.2 to 1.5.4 (FreeTubeApp#4398)
  Bump electron from 27.1.0 to 27.1.2 (FreeTubeApp#4395)
  Bump electron-builder from 24.6.4 to 24.9.1 (FreeTubeApp#4397)
  Translated using Weblate (Portuguese)
  Translated using Weblate (French)
  Fix bug (FreeTubeApp#4392)
  Translated using Weblate (Spanish)
  Translated using Weblate (Chinese (Traditional))
  Translated using Weblate (Serbian)
  Translated using Weblate (Chinese (Simplified))
  Translated using Weblate (Italian)
  Cleanup SponsorBlock video id hashing (FreeTubeApp#4384)
  ...

# Conflicts:
#	src/renderer/scss-partials/_ft-list-item.scss
@PikachuEXE PikachuEXE mentioned this pull request Dec 11, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PikachuEXE PikachuEXE PikachuEXE approved these changes

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc approved these changes

@kommunarr kommunarr kommunarr approved these changes

@absidue absidue Awaiting requested review from absidue

@ChunkyProgrammer ChunkyProgrammer Awaiting requested review from ChunkyProgrammer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@MitchelPaulin @kommunarr @shadycloud @absidue @efb4f5ff-1298-471a-8973-3d47447115dc @PikachuEXE @FreeTubeBot