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 support for markdown.nvim | changed some native markup/down highlights TBD if final #154

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

avih7531
Copy link
Contributor

@avih7531 avih7531 commented Aug 4, 2024

Added support for markdown.nvim

Had to change multiple @markup highlight groups because of preset bolds and italics, to get a nice looking RenderMarkdown, but think it came out great.

Any thoughts on whether or not to keep the bolds for H1-3 and italics for H5-6 permanently? If so, should they persist in the RenderMarkdown groups? My vote would be no as I'm unfamiliar with the convention for highlights like that, but then again, not my repo :)

AH

@avih7531
Copy link
Contributor Author

avih7531 commented Aug 4, 2024

08-04-2024_19:51:56 253
08-04-2024_19:52:11 537
08-04-2024_19:52:28 011

Copy link
Collaborator

@5-pebbles 5-pebbles left a comment

Choose a reason for hiding this comment

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

Thanks for the screenshots. I should note this is not my repository either, the following review is just my opinion.

lua/nordic/groups/integrations.lua Outdated Show resolved Hide resolved
lua/nordic/groups/integrations.lua Outdated Show resolved Hide resolved
@@ -348,17 +349,17 @@ function M.get_groups()
G['@text.note'] = { link = 'Note' }
--- Markup
G['@markup'] = { link = '@none' }
G['@markup.emphasis'] = { italic = true }
G['@markup.emphasis'] = { fg = C.orange.base, italic = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see why you did this, and I think it makes it easier to edit the rendering (by making those things noticeable). However, I think it should just occur on rendered text because it is distracting otherwise.

Copy link
Contributor Author

@avih7531 avih7531 Aug 5, 2024

Choose a reason for hiding this comment

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

Fair enough. That might just be a personal preference, but I noticed it adds so much to a markdown if emphasis, bold, and italic are different colors. Will see if I can make it only apply to rendered text

G['@markup.italic'] = { italic = true }
G['@markup.heading.5'] = { fg = C.blue2 }
G['@markup.heading.6'] = { fg = C.cyan.base }
G['@markup.italic'] = { fg = C.orange.base, italic = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as @markup.emphasis

Comment on lines -373 to +374
G['@markup.strong'] = { bold = true }
G['@markup.strong'] = { fg = C.orange.base, bold = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as @markup.emphasis

lua/nordic/groups/native.lua Outdated Show resolved Hide resolved
@5-pebbles 5-pebbles changed the base branch from main to dev August 5, 2024 15:45
@avih7531
Copy link
Contributor Author

avih7531 commented Aug 5, 2024

Appreciate the code review, by the way -- I've never had one. Will get back later today with a revised version

@5-pebbles
Copy link
Collaborator

5-pebbles commented Aug 5, 2024

Your welcome; make commits for bold headers (with relinking markdownH1 + markdownH2) and the background blend. I would leave the orange emphasis for Alex to look at, though.

@5-pebbles
Copy link
Collaborator

5-pebbles commented Aug 6, 2024

Here are some examples of the orange and not orange markdown section for comparison:

Example 1

image
image

Example 2

image
image

Example 3 (with rendering)

image
image

@avih7531
Copy link
Contributor Author

avih7531 commented Aug 8, 2024

Just committed changes to 'avih7531:nordic.nvim' hopefully that updated here -- sorry not so good with github!!

@5-pebbles
Copy link
Collaborator

Just committed changes to 'avih7531:nordic.nvim' hopefully that updated here -- sorry not so good with github!!

I am assuming you are talking about the one three days ago: 381b17c If not, you still need to push your changes.

I had never worked with github before helping with this repo, so don't worry about inexperience. Also, you should be able to see all commits on the Commits tab of this page (at the top) and all changes in the Files changed tab.

@avih7531
Copy link
Contributor Author

avih7531 commented Aug 8, 2024

Yes! Commit 381b17c should be what was asked before hand. Changed all headings to bold, re-linked markdownH1&2, and fixed the C.bg blend. Left italics and bolds as is for Alex to decide.

Appreciate your patience and support working with Git! Definitely new to me, but learning.

@AlexvZyl
Copy link
Owner

@5-pebbles Please resolve conversations that you are happy with :)

@5-pebbles
Copy link
Collaborator

5-pebbles commented Aug 17, 2024

Alright, @avih7531 I am going to ask that you revert the orange emphasis. I have been using it since this PR was opened, and I still find it a little distracting. It's also fairly easy to add with the new on_highlight option.

I will let you deal with the merge conflict. If you are new to git, it will be a good practice.

@avih7531
Copy link
Contributor Author

Thanks both! Will have that done by tonight -- appreciate giving me some git practice, too

@AlexvZyl
Copy link
Owner

Ping @avih7531

@5-pebbles
Copy link
Collaborator

@avih7531 (•ᴥ• )́ If you would like I could finish this pr...

@AlexvZyl
Copy link
Owner

This will have to get added in a different release.

@AlexvZyl AlexvZyl deleted the branch AlexvZyl:main September 26, 2024 13:32
@AlexvZyl AlexvZyl closed this Sep 26, 2024
@AlexvZyl AlexvZyl reopened this Sep 26, 2024
@AlexvZyl AlexvZyl deleted the branch AlexvZyl:main September 28, 2024 09:16
@AlexvZyl AlexvZyl closed this Sep 28, 2024
@AlexvZyl AlexvZyl reopened this Sep 28, 2024
@AlexvZyl AlexvZyl changed the base branch from dev to main September 28, 2024 16:13
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