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

Fix: State #113

Merged
merged 5 commits into from
Mar 23, 2024
Merged

Fix: State #113

merged 5 commits into from
Mar 23, 2024

Conversation

AlexvZyl
Copy link
Owner

@AlexvZyl AlexvZyl commented Feb 28, 2024

This brings a major refactor to how state and colors are managed.

@5-pebbles take a look.

Closes #112.
Closes #108.
Closes #99.

@AlexvZyl AlexvZyl added the Enhancement New feature, request or suggestion label Feb 28, 2024
@AlexvZyl AlexvZyl self-assigned this Feb 28, 2024
@AlexvZyl
Copy link
Owner Author

@5-pebbles a lot has changed, probably easier to check the actual branch.

@AlexvZyl
Copy link
Owner Author

@5-pebbles Seems like the tests are working now.

@AlexvZyl
Copy link
Owner Author

AlexvZyl commented Feb 28, 2024

Let's get this sorted @5-pebbles, then I will add you as a contributor so that you can create branches on this repository.

@5-pebbles
Copy link
Collaborator

My opinions on this:

  1. In colors/init.lua: Lua modules are only ran once so you can just call C.extend_palette() no need for C.extended.

  2. In colors/init.lua: local O = require 'nordic.config'.options should be local options like in my fork, capital letters usually denote global variables.

  3. In colors/init.lua: C.apply_modifications seems weird seeing as most modifications are happening in C.extend_palette anyway. For example the handling of transparent_bg, bg_highlight, and bright_border.

  4. In colors/init.lua: swap backgrounds is still a toggle...

  5. In colors/init.lua: reduce blue is still one directional so reloading the config there wont work.

  6. In colors/init.lua: extend_palette() & apply_modifications() are too statefull for me, the outcome can be changed by which order you call them in. It requires more developer knowledge.

I have not looked at the rest, and I don't want to be rude but I think my fork does all of these better are you sure you don't want me to make a pr?

@5-pebbles
Copy link
Collaborator

5-pebbles commented Feb 28, 2024

I do like 6b417a4 but the rest still seems messy.

#114 I think it does a better job on colors/init.lua but I like your changes to groups and I like the local imports in lua/nordic/init.lua however I think they should be lower case...

@AlexvZyl
Copy link
Owner Author

@5-pebbles Once you have accepted the invite, could you check for me if you are able to bypass the branch protection rules?

@5-pebbles
Copy link
Collaborator

@5-pebbles Once you have accepted the invite, could you check for me if you are able to bypass the branch protection rules?

How would I do that?

@AlexvZyl
Copy link
Owner Author

@5-pebbles Once you have accepted the invite, could you check for me if you are able to bypass the branch protection rules?

How would I do that?

If you can bypass it you should be able to tick a box above the merge button that says "bypass". But seems like you can't so don't worry.

@5-pebbles
Copy link
Collaborator

yup there is no such button for me.

@5-pebbles
Copy link
Collaborator

Alright I am done: #115

Co-authored-by: Alexander van Zyl <calexandervanzyl@gmail.com>
@5-pebbles
Copy link
Collaborator

What the plan for this pr is there anything that needs to be done?

@AlexvZyl
Copy link
Owner Author

I think all we need to do is double check that #99, #108 and #112 is resolved.

Then we can merge.

@AlexvZyl
Copy link
Owner Author

There are some other things we need to change, but we can do that in a different PR.

@5-pebbles
Copy link
Collaborator

I think all we need to do is double check that #99, #108 and #112 is resolved.

Then we can merge.

I had already checked and confirmed that #108 & #112 are fixed.

I just checked #99:

require 'nordic'.setup({
  override = {
    NeoTreeGitAdded = {
      fg = palette.orange.base, -- this is fixed
    },
  },
  on_palette = function(nordicPalette)
    nordicPalette.git.add = '#FFFFFF' -- and this does not work but I don't think its supposed to...
    -- it could work with a after_palette function which you talked about adding in a different pr though
    return nordicPalette
  end,
})

@5-pebbles
Copy link
Collaborator

The conflict was caused by the changes made in #117 but everything should be good to go now.

@AlexvZyl
Copy link
Owner Author

LGTM.

@AlexvZyl AlexvZyl merged commit 5e2813e into main Mar 23, 2024
1 check passed
@5-pebbles 5-pebbles deleted the fix/state branch April 10, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature, request or suggestion
Projects
None yet
2 participants