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

added variable to make doom-modeline optional #45

Closed
wants to merge 6 commits into from
Closed

added variable to make doom-modeline optional #45

wants to merge 6 commits into from

Conversation

erikLundstedt
Copy link
Contributor

this closes issue #44

Signed-off-by: Erik Lundstedt erik@lundstedt.it

this closes issue #44

Signed-off-by: Erik Lundstedt <erik@lundstedt.it>
modules/rational-ui.el Outdated Show resolved Hide resolved
modules/rational-ui.el Outdated Show resolved Hide resolved
erikLundstedt and others added 3 commits February 18, 2022 19:44
Co-authored-by: Mathieu Marques <mathieumarques78@gmail.com>
Co-authored-by: Mathieu Marques <mathieumarques78@gmail.com>
@daviwil
Copy link
Member

daviwil commented Feb 19, 2022

Choice of mode line customization is a pretty big one, on the order of choosing your preferred font and color theme.

I'm starting to wonder if we should configure the mode line somewhere else, like in a separate module that can be included only if you want mode line customizations. That would make it easier to have multiple rational-doom-modeline.el etc configurations which don't require configuration variables to decide whether they're turned on or off.

What do you think?

@erikLundstedt
Copy link
Contributor Author

Choice of mode line customization is a pretty big one, on the order of choosing your preferred font and color theme.

I'm starting to wonder if we should configure the mode line somewhere else, like in a separate module that can be included only if you want mode line customizations. That would make it easier to have multiple rational-doom-modeline.el etc configurations which don't require configuration variables to decide whether they're turned on or off.

What do you think?

I don't know

I was planning on making a pr with the telephone-line modeline config I've got where you run a function to use different configurations/defaults and the user get to start the modeline themselves as you need to do the config before starting the modeline with telephone-line-mode or something similar

@erikLundstedt
Copy link
Contributor Author

Choice of mode line customization is a pretty big one, on the order of choosing your preferred font and color theme.

I'm starting to wonder if we should configure the mode line somewhere else, like in a separate module that can be included only if you want mode line customizations. That would make it easier to have multiple rational-doom-modeline.el etc configurations which don't require configuration variables to decide whether they're turned on or off.

What do you think?

Actually
That sounds pretty good
Because then I can write (parts of) the telephone-line module

Copy link
Contributor

@bkaestner bkaestner left a comment

Choose a reason for hiding this comment

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

The current commit history is a bit of a mess. While the intend is fine, can you please clean it up? Given the contents of your changes, you could squash them into a single commit.

(customize-set-variable 'doom-modeline-bar-width 6)
(customize-set-variable 'doom-modeline-minor-modes t)
(customize-set-variable 'doom-modeline-buffer-file-name-style 'truncate-except-project)
(defcustom rational-ui-use-doom-modeline t
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't concur with setting a variable that controls an optional feature to t by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't concur with setting a variable that controls an optional feature to t by default.

thats to not break existing configs
i could change that i guess

@erikLundstedt
Copy link
Contributor Author

The current commit history is a bit of a mess. While the intend is fine, can you please clean it up? Given the contents of your changes, you could squash them into a single commit.

I am not sure on how to do that as ive only ever used git(gitlab) for my own code where messy history isnt a problem

@angrybacon
Copy link

angrybacon commented Feb 19, 2022

I am not sure on how to do that as ive only ever used git(gitlab) for my own code where messy history isnt a problem

It is always an issue, it's just that GitHub (and GitLab) will optionally let you squash commits while merging once the PR is accepted. That being said, squashing yourself is the conventional thing to do and it helps codeowners review changes more easily without carrying the minor details we talked about here

To squash your commits into one, you can pull your branch locally as per latest, and if you're using Magit, ri from the status buffer is probably the easiest

@erikLundstedt
Copy link
Contributor Author

I am not sure on how to do that as ive only ever used git(gitlab) for my own code where messy history isnt a problem

It is always an issue, it's just that GitHub (and GitLab) will optionally let you squash commits while merging once the PR is accepted. That being said, squashing yourself is the conventional thing to do and it helps codeowners review changes more easily without carrying the minor details we talked about here

To squash your commits into one, you can pull your branch locally as per latest, and if you're using Magit, ri from the status buffer is probably the easiest

the thing now is that there have been changes made afterwards

im gonna try

This pull request was closed.
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.

None yet

4 participants