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

feat(style): new style proposal #1948

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

DenysMb
Copy link
Contributor

@DenysMb DenysMb commented Oct 26, 2024

Pre-flight Checklist

Please ensure you've completed all of the following.

Description of Change

Changed the tab style and added paddings and border-radius in the service view (this one has this separated PR in if you like the idea of having these paddings but not changing the tab style #1947).

Motivation and Context

New applications like Arc Browser (screenshot 1 and screenshot 2) have been using this style of buttons in the sidebar with paddings, gaps and border-radius. I think that this would give Ferdium a new modern yet minimalist style.

Screenshots

Screenshot_20241026_192715
Screenshot_20241026_192725
Screenshot_20241026_192813
Screenshot_20241026_192825
Screenshot_20241026_192910
Screenshot_20241026_192928
Screenshot_20241026_192940
Screenshot_20241026_192955

Checklist

  • My pull request is properly named
  • The changes respect the code style of the project (pnpm prepare-code)
  • pnpm test passes
  • I tested/previewed my changes locally

Release Notes

Changed the tab style and added paddings and border-radius in the service view.

Copy link
Contributor

@vraravam vraravam left a comment

Choose a reason for hiding this comment

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

LGTM - but will wait for 1 more core contributor to provide their opinions before this can be merged
@SpecialAro / @Alphrag - wdyt?

@vraravam vraravam mentioned this pull request Oct 27, 2024
6 tasks
@SpecialAro
Copy link
Member

LGTM as well, but I'll suggest to add a setting option to change the padding (so users can remove the padding or adjust it according to their needs)

@DenysMb
Copy link
Contributor Author

DenysMb commented Oct 27, 2024

LGTM as well, but I'll suggest to add a setting option to change the padding (so users can remove the padding or adjust it according to their needs)

Alright, makes sense. I'll start to do this change.

Do you prefer:

  • An option to show/hide the padding;
  • An option with a select input with four values: None (0px), Small (4px), Medium (8px) and Large (16px);
  • An option with a number input where the user can set any value for the padding that he wants.

The first one is what I would choose. It doesn't give the user the option to set another value, but it will give the option to show or hide. It is most simple and it would be easy to maintain or to other contributors to understand.

If we really want to give other options to the user, I would prefer the second option as it will give us the control of the values, it would be easier to maintain and prevent issues that could appear by letting the user select any value for the paddings.

@DenysMb
Copy link
Contributor Author

DenysMb commented Oct 28, 2024

This would be the first option:

Screencast_20241028_110918.webm

@vraravam
Copy link
Contributor

@DenysMb - please rebase and fix conflicts. btw - is this PR still needed/valid?

@DenysMb
Copy link
Contributor Author

DenysMb commented Nov 12, 2024

@DenysMb - please rebase and fix conflicts. btw - is this PR still needed/valid?

It is more a proposal than something that is needed. You can close this PR.
I will keep my fork updated with changes from master and this new style so I can use it for my personal use.

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