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

fixed the spacing issue in primary key and similar combos #4106

Merged
merged 6 commits into from
Sep 30, 2024

Conversation

Dxuian
Copy link
Contributor

@Dxuian Dxuian commented Aug 30, 2024

Resolves #4057

before
image

after
image

Checklist

  • Added markup tests, or they don't apply here because...
  • Updated the changelog at CHANGES.md

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

5 files changed

Total change +5 B

View Changes
file base pr diff
es/core.min.js 8.18 KB 8.18 KB +1 B
es/highlight.min.js 8.18 KB 8.18 KB +1 B
es/languages/sql.min.js 2.44 KB 2.44 KB +1 B
highlight.min.js 8.22 KB 8.22 KB +1 B
languages/sql.min.js 2.44 KB 2.44 KB +1 B

@Dxuian Dxuian mentioned this pull request Sep 1, 2024
1 task
Copy link

github-actions bot commented Sep 4, 2024

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

5 files changed

Total change -1 B

View Changes
file base pr diff
es/core.min.js 8.18 KB 8.18 KB -1 B
es/highlight.min.js 8.18 KB 8.18 KB -1 B
es/languages/sql.min.js 2.44 KB 2.44 KB +1 B
highlight.min.js 8.22 KB 8.22 KB -1 B
languages/sql.min.js 2.44 KB 2.44 KB +1 B

Copy link

github-actions bot commented Sep 5, 2024

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

3 files changed

Total change +3 B

View Changes
file base pr diff
es/languages/sql.min.js 2.44 KB 2.44 KB +1 B
highlight.min.js 8.22 KB 8.22 KB +1 B
languages/sql.min.js 2.44 KB 2.44 KB +1 B

@joshgoebel
Copy link
Member

Could you please test my iteration? It passes all your tests - as well as adding support for multi-word type modifiers also (for flexible spacing).

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

5 files changed

Total change +17 B

View Changes
file base pr diff
es/core.min.js 8.18 KB 8.18 KB -1 B
es/highlight.min.js 8.18 KB 8.18 KB -1 B
es/languages/sql.min.js 2.44 KB 2.45 KB +10 B
highlight.min.js 8.22 KB 8.22 KB -1 B
languages/sql.min.js 2.44 KB 2.45 KB +10 B

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

5 files changed

Total change +10 B

View Changes
file base pr diff
es/core.min.js 8.18 KB 8.18 KB -2 B
es/highlight.min.js 8.18 KB 8.18 KB -2 B
es/languages/sql.min.js 2.44 KB 2.45 KB +8 B
highlight.min.js 8.22 KB 8.22 KB -2 B
languages/sql.min.js 2.44 KB 2.45 KB +8 B

@Dxuian
Copy link
Contributor Author

Dxuian commented Sep 23, 2024

Could you please test my iteration? It passes all your tests - as well as adding support for multi-word type modifiers also (for flexible spacing).

i tested your code and it ran into problems as it started identifying comments as non-commented code
but i understood your apporach and integrated it with the commits i made
your approach with comments
your approach:
image
highlighted comment:
image
updated approach:
image
doesnt highlight comments apporach:
image

@Dxuian
Copy link
Contributor Author

Dxuian commented Sep 23, 2024

ive made a commit regarding the fixes pls review @joshgoebel

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

5 files changed

Total change +42 B

View Changes
file base pr diff
es/core.min.js 8.18 KB 8.18 KB +2 B
es/highlight.min.js 8.18 KB 8.18 KB +2 B
es/languages/sql.min.js 2.44 KB 2.46 KB +18 B
highlight.min.js 8.22 KB 8.22 KB +2 B
languages/sql.min.js 2.44 KB 2.46 KB +18 B

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

2 files changed

Total change +36 B

View Changes
file base pr diff
es/languages/sql.min.js 2.44 KB 2.46 KB +18 B
languages/sql.min.js 2.44 KB 2.46 KB +18 B

@joshgoebel
Copy link
Member

joshgoebel commented Sep 24, 2024

Simple typo... I think you're using another grammar entirely... here is what I have now:

Screen Shot 2024-09-23 at 10 46 52 PM

Looks right to me.

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

3 files changed

Total change -1 B

View Changes
file base pr diff
es/languages/sql.min.js 2.44 KB 2.44 KB -1 B
highlight.min.js 8.22 KB 8.22 KB +1 B
languages/sql.min.js 2.44 KB 2.44 KB -1 B

@joshgoebel
Copy link
Member

Now I'm thinking some of these should be types (since they modify the type), not keywords... such as:

"primary key",
"foreign key",
"not null",

@Dxuian
Copy link
Contributor Author

Dxuian commented Sep 29, 2024

Simple typo... I think you're using another grammar entirely... here is what I have now:

Screen Shot 2024-09-23 at 10 46 52 PM Looks right to me.

Simple typo... I think you're using another grammar entirely... here is what I have now:

Screen Shot 2024-09-23 at 10 46 52 PM Looks right to me.

apologies i changed to a different style and thought comment were being highlighted wrong

@joshgoebel
Copy link
Member

So all good now?

@Dxuian
Copy link
Contributor Author

Dxuian commented Sep 30, 2024

yes

Copy link

Build Size Report

Changes to minified artifacts in /build, after gzip compression.

3 files changed

Total change -3 B

View Changes
file base pr diff
es/languages/sql.min.js 2.44 KB 2.44 KB -1 B
highlight.min.js 8.22 KB 8.22 KB -1 B
languages/sql.min.js 2.44 KB 2.44 KB -1 B

@joshgoebel joshgoebel merged commit ff3985d into highlightjs:main Sep 30, 2024
19 checks passed
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.

(sql) 'foreign key' and 'primary key' do not highlight 'key'.
2 participants