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

Feature/abyss boreal colorscheme #50

Conversation

egotch
Copy link
Contributor

@egotch egotch commented Jan 4, 2025

#49

Abyss-Boreal theme variation

Color Palette

Cool tones inspired by the norther lights and the Minnesota North Woods.

  • Color palette for abyss and abyss-boreal updated to be more granular (added individual entries on the palette.lua files for extensions such as Aerial, NeoTree, and the Terminal

Extensions

  • Support for Aerial
  • Support for NeoTree (adding additional line items on the palette for icon, text, and current line underline)
  • Support for Lualine with individual mappings on the palette.lua

Documentation

I left some placeholders for the documentation. I don't have access to the assets on the main repo, so not sure how we want to handle that?? Let me know if you want me to get a screenshot on there.

- adding functionality for NeoTree support
 - adding Aerial support
 - breaking down colors to be more granular for future setting
- NeoTree support (underline highlighting)
 - Aerial support (underline highlighting)
- adding palette setting for abyss-boreal
 - adding palette setting for original abyss
 - confirmed can swap between themese successfully!
- was using a copied version for simplified testing
 - replacing to original now that can swap between themes
- moved palette to its own file
 - added lualine color spec
- darkening the primary foreground
 - cleaning up unused colors
 - setting selection to the same darkred as abyss
- matching terminal colors from the mystic night theme
- adding some placeholders for screenshots of the original abyss and
abyss-boreal themes
 - just placeholders since I don't have access to the same assets as the
repo owner
- was too close to cursor color, couldn't tell where you were when on a
parenthesis/bracket/curly-brace
autoload/airline/themes/abyss-boreal.vim Outdated Show resolved Hide resolved
autoload/lightline/colorscheme/abyss-boreal.vim Outdated Show resolved Hide resolved
test/abyss/palette_spec.lua Show resolved Hide resolved
@barrientosvctor barrientosvctor added the enhancement New feature or request label Jan 4, 2025
@barrientosvctor
Copy link
Owner

barrientosvctor commented Jan 4, 2025

@egotch Btw I've got a question, should the CursorLine highlight group looks like this?

image

If so, the colour makes difficult to read the code with soft colours.

Edit: I don't know if the Cursorline colour was meant to look like this (Hex color: #282049):

image

If so, maybe you got confused following these comments

image

Those comments are outdated and actually they're not being used for what it says, that's my fault. The up-to-date comments are in types.lua file, if you want your variant looks better, you should follow those ones.

@egotch egotch force-pushed the feature/abyss-boreal-colorscheme branch from c80dd3c to 61c1db3 Compare January 5, 2025 02:42
@egotch
Copy link
Contributor Author

egotch commented Jan 5, 2025

@egotch Btw I've got a question, should the CursorLine highlight group looks like this?

image

If so, the colour makes difficult to read the code with soft colours.

Edit: I don't know if the Cursorline colour was meant to look like this (Hex color: #282049):

image

If so, maybe you got confused following these comments

image

Those comments are outdated and actually they're not being used for what it says, that's my fault. The up-to-date comments are in types.lua file, if you want your variant looks better, you should follow those ones.

Thanks for that and good catch there. I have the cursor line highlighting disabled on my vim setup, so didn't catch that.

@egotch egotch closed this Jan 5, 2025
@egotch egotch reopened this Jan 5, 2025
@egotch
Copy link
Contributor Author

egotch commented Jan 5, 2025

accidentally clicked close there

@egotch egotch requested a review from barrientosvctor January 5, 2025 02:59
@barrientosvctor
Copy link
Owner

accidentally clicked close there

don't worries 👍

@egotch
Copy link
Contributor Author

egotch commented Jan 5, 2025

All requested changes should now be in place

Copy link
Owner

@barrientosvctor barrientosvctor left a comment

Choose a reason for hiding this comment

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

Your changes was good, but these things I reviewed are missing to work correctly.

autoload/lightline/colorscheme/abyss-boreal.vim Outdated Show resolved Hide resolved
autoload/airline/themes/abyss-boreal.vim Outdated Show resolved Hide resolved
@barrientosvctor
Copy link
Owner

@egotch All your changes works now.

Talking about screenshots for abyss variants, could you take a screenshot to add it then in the readme?

@egotch
Copy link
Contributor Author

egotch commented Jan 5, 2025

just tested that I could run both of those commands:

image

@egotch egotch requested a review from barrientosvctor January 5, 2025 03:33
@egotch
Copy link
Contributor Author

egotch commented Jan 5, 2025

@egotch All your changes works now.

Talking about screenshots for abyss variants, could you take a screenshot to add it then in the readme?

You bet. I'll get some good ones

Copy link
Owner

@barrientosvctor barrientosvctor left a comment

Choose a reason for hiding this comment

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

Your changes works successfully, however, tests are still failing. That's because the properties of the specs has changed and are not actually included. That's something that I can do after merging the PR.

Another things I'm gonna do after merging the PR is:

  • Refactor the palette test file
  • Add the new spec properties in types.lua file and document them
  • Apply the new properties into abyss palette
  • Fix a non-PR related Vim bug
  • Add the preview images (yours) for each abyss variant.

After doing all these things I'm gonna merge everything to main branch.

Meanwhile, you can continue adding more things that you consider are missing before merging. When everything is done, tell me to merge the PR. 👍

Comment on lines +101 to +105
dirtree = {
rootname = palette.white,
dirname = palette.darkteal,
curline = palette.cerulean,
},
Copy link
Owner

Choose a reason for hiding this comment

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

I've got a question, can these properties be used for any another file explorer-related plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea you can if you'd like to! I just set them for Neotree because it has that level of granularity on highlight groups

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea you can if you'd like to! I just set them for Neotree because it has that level of granularity on highlight groups. But no reason we couldn't use the same on the other file explorer extensions.

Copy link
Owner

Choose a reason for hiding this comment

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

I'll implement those properties in the highlight groups of other plugins like Neotree later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@barrientosvctor
readme is updated with previews. After you merge, you'll need to update the urls of the preview images, otherwise they'll continue to point to my fork.

but you can preview it here:
https://github.com/egotch/abyss.nvim/tree/feature/abyss-boreal-colorscheme

looks pretty decent I'd say 😄

Copy link
Owner

Choose a reason for hiding this comment

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

@barrientosvctor readme is updated with previews. After you merge, you'll need to update the urls of the preview images, otherwise they'll continue to point to my fork.

Okay, I'll take care about that.

looks pretty decent I'd say 😄

They look amazing! Look so professional tbh.

When you consider everything is finished, tell me to merge the PR.

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 say we're ready, no additional changes from my end. MERGE!! 😎

@barrientosvctor barrientosvctor merged commit 7e77587 into barrientosvctor:develop Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants