-
Notifications
You must be signed in to change notification settings - Fork 56
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: Add orientation support for clock, button, label, and sys_info #530
Conversation
Just noticed there are a few module's I'm not using that currently still rotate based on bar position (upower, music, sysinfo) so I'd be happy to add orientation support for those as well if you like. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for the PR. I really appreciate the work you've put in here. Vertical bars have needed some love for a very long time so I'm very glad to finally see them get some.
The code generally lgtm, there's a few picky points I've raised but nothing of actual concern, other than the build failures.
Can you also ensure your commits follow the convention commits standard please, so that they get included in the changelog (and you get credited)
If you want to take these on too, I'd really appreciate it as well. No pressure if you're not up for it though :) |
0b245d0
to
6aa7455
Compare
I addressed the PR feedback and I added orientation support for sysinfo. I took a stab at upower and music but I got hung up on rotating the icons (if you know how to do this I'd be happy to do the rest). Notably right now upower at least doesn't really have proper behavior on vertical bars since only the label changes when the whole button needs to change. IMO it'd be better to just remove line 184 there, you'd get a more functional widget in vertical mode. Line 184 in 4a3bca1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more small changes please, and then if you can squash your commits down to remove the PR-related noise I think we're getting there.
I took a stab at upower and music but I got hung up on rotating the icons (if you know how to do this I'd be happy to do the rest).
Sure, looking at re-doing the icon code is actually next on my agenda as part of #511 anyway so I can pick that up.
Feel free to remove that line from UPower in the meantime.
@JakeStanger Resolved all your feedback. Is there a particular reason you want me to squash my commits manually instead of using Github's squash merge feature? |
GH's squash merge will reduce everything into a single commit, I just want to filter out the PR-related commits :) There's still several changes this PR makes so I'd like to keep them separated so it's clear in the changelog:
|
Gotcha, I'll try to get it down to relevant commits. It does mean that the commits themselves won't be strictly what is mentioned in their commit messages since interactive rebase can only squash into adjacent commits. |
You can actually squash into any commit! You can re-order the lines in the editor to change what they're adjacent to, and it should work it out. If you want a hand with that let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code all LGTM - if you can just fix formatting too pls, and then we're there. Cheers for your hard work on this
I don't have a separate commit for actually introducing the test config but I'll preserve the commit where I converted it to corn. |
Ok, I reworked my commits. I've gotten used to squash merges but in the future I'll try to maintain better commit hygiene on PRs for ironbar. |
I appreciate everyone works differently and I know it's quite an involved way of working compared to a lot of repos, so appreciate the extra effort. It's mostly me being lazy so I don't have to write the changelog myself, but I do think it helps keep commits tidy and sensibly sized, which is so very useful when I need to go back through the history. All boxes ticked, commits look good, code all LGTM now so will merge. Cheers again :) |
@JakeStanger Thanks for all the help and the quick merge! |
After making the switch to hyprland ironbar is by far the best dock solution I've found. Most wayland dockbars that aren't built into a big DE have very poor support for vertical docks or lack a proper program list/launcher. The one big issue I've run into is that the clock is incorrectly rotated when using a vertical bar. In this discussion you mentioned you wanted to keep the current behavior as an option.
I went ahead and added orientation support for clock as well as label and button which some people were asking for. I also created an enum for orientation and moved deserialization into serde and out of an ad-hoc function.
I also added a test config where you can see that all variations (default, horizontal, and vertical along with shorthand) are functioning properly.
Please let me know if there's anything you need on my end to get this merged, I didn't see any contribution guidelines or standards in the repo.