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(bar): add two new display format types #1176

Merged

Conversation

alex-ds13
Copy link
Contributor

@alex-ds13 alex-ds13 commented Dec 14, 2024

This commit adds two new DisplayFormat types:

  • TextAndIconOnSelected: which displays icon and text for the focused element and the other elements only have text.
  • IconAndTextOnSelected: which displays icon and text for the focused element and the other elements only have icon.

Here's a picture that shows both types in action, the workspaces widget has the TextAndIconOnSelected and the focused window has the IconAndTextOnSelected:

image

Currently I've only setup this new types to the workspaces and focused_window widgets. Any other widget that uses DisplayFormat is probably ignoring these new types. If you find there is any other widget that makes sense to handle these types tell me and I can try adding them or they can be added later if someone asks for it.

@alex-ds13 alex-ds13 force-pushed the feature/bar-display-icon-text-on-focused branch from 82ab501 to 8950098 Compare December 14, 2024 13:02
@CtByte
Copy link
Contributor

CtByte commented Dec 14, 2024

This was on my TODO as well :D

@alex-ds13 alex-ds13 force-pushed the feature/bar-display-icon-text-on-focused branch from 8950098 to e4c7350 Compare December 14, 2024 17:18
@alex-ds13
Copy link
Contributor Author

This was on my TODO as well :D

One less TODO! 😄 Do you think this new display types need to be handled on any other widget, besides those two?

@CtByte
Copy link
Contributor

CtByte commented Dec 14, 2024

I think it makes very good sense on these 2 (saving space) and it can be added to others later (but no other right now that would need it) 👍

@alex-ds13 My only comment on this PR would be to name the new options TextAndIconOnSelected and IconAndTextOnSelected, since you are setting whether or not a SelectableFrame is selected. What do you think?

@alex-ds13 alex-ds13 force-pushed the feature/bar-display-icon-text-on-focused branch from e4c7350 to 0874a1b Compare December 15, 2024 00:17
@alex-ds13
Copy link
Contributor Author

@alex-ds13 My only comment on this PR would be to name the new options TextAndIconOnSelected and IconAndTextOnSelected, since you are setting whether or not a SelectableFrame is selected. What do you think?

Makes sense to me! I'll change that...

@alex-ds13 alex-ds13 force-pushed the feature/bar-display-icon-text-on-focused branch from 0874a1b to 81d8638 Compare December 17, 2024 16:41
@alex-ds13
Copy link
Contributor Author

@LGUG2Z As mentioned on Discord, I've rebased this PR on top of #1177, this way you can merge this PR's without conflicts since all three PR's were changing things on the same files. This way the final order to merge should be the following:

  1. feat(bar): scaled icon size relying on the font size #1184 (from @CtByte - makes icons have fixed sizes, which fixes a few issues, and adds some scaling to the icons)
  2. feat(bar): optional workspaces on Komorebi widget #1177 (from me - makes the workspaces optional on Komorebi widget so that we can have workspaces on left_widgets and focus_window on center_widgets for example...)
  3. feat(bar): add two new display format types #1176 (this PR - adds the display formats TextAndIconOnSelected and IconAndTextOnSelected)

@LGUG2Z
Copy link
Owner

LGUG2Z commented Dec 17, 2024

Ah sorry, this might need another rebase; I updated the icon size commit message to include the context that was on the PR

@alex-ds13 alex-ds13 force-pushed the feature/bar-display-icon-text-on-focused branch from 81d8638 to abe5871 Compare December 17, 2024 22:30
@alex-ds13
Copy link
Contributor Author

Ah sorry, this might need another rebase; I updated the icon size commit message to include the context that was on the PR

Should be good to go after #1177

@LGUG2Z
Copy link
Owner

LGUG2Z commented Dec 17, 2024

Weird, I just merged in #1177 without touching it and now this one is again saying there are conflicts 🤔

This commit adds two new `DisplayFormat` types:
- `TextAndIconOnSelected`: which displays icon and text for the selected
  element and the other elements only have text.
- `IconAndTextOnSelected`: which displays icon and text for the selected
  element and the other elements only have icon.
@alex-ds13 alex-ds13 force-pushed the feature/bar-display-icon-text-on-focused branch from abe5871 to 0120725 Compare December 17, 2024 23:31
@alex-ds13
Copy link
Contributor Author

Weird, I just merged in #1177 without touching it and now this one is again saying there are conflicts 🤔

Weird I couldn't even rebase it to master since it considered it a noop and trying to force push would tell me that there was nothing to push. I guess the reason was that the commit on this PR had a different hash than the one on master... Although that's usually not a problem, at least not locally to me since when rebasing it can tell that they are the same commit and simply shows a message that it skipped a cherry-picked commit...

Maybe github can't or doesn't allow or isn't configured to do the same...

But now it should work.

@LGUG2Z LGUG2Z merged commit d393f8f into LGUG2Z:master Dec 18, 2024
6 checks passed
@alex-ds13 alex-ds13 deleted the feature/bar-display-icon-text-on-focused branch December 18, 2024 00:34
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