Skip to content

Conversation

mkslanc
Copy link
Contributor

@mkslanc mkslanc commented Jun 27, 2025

Issue #, if available:

Description of changes:
This PR introduces a robust text markers implementation, enabling customized text highlighting. The feature includes a clean API, efficient rendering, and support for multiline selections.

Key Features

  • Added text markers API for editor sessions
  • Implemented DOM manipulation for text highlighting
  • Added scheduling system to automatically remove outdated markers
  • Enhanced search box functionality with selection highlighting (for kitchen-sink as a demo)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Pull Request Checklist:

@mkslanc mkslanc force-pushed the color-change-selection branch from 59d501e to b8dd235 Compare June 27, 2025 12:00
@mkslanc mkslanc force-pushed the color-change-selection branch from b8dd235 to c320c30 Compare June 27, 2025 12:01
@marinsokol5
Copy link
Contributor

Hey, great job on the initial draft! 🚀

Is there a nice way to trigger it today in the kitchen-sink or somewhere, or would one have to invoke the addTextMarker manually?
I am very curious to see how the UI would look like with this partial styling of certain tokens, where "mySt" is colored green, "rin" colored as text, and then "g" as green again.

Did you give any thought on how this will be introduced? Would something like a general editor option "disableSelectionHighlight" make sense, or is too specific, and we would maybe apply these text markers by default when there is a selection and let consumers handle it through CSS?

@mkslanc
Copy link
Contributor Author

mkslanc commented Jun 30, 2025

@marinsokol5 Hey! I have small kitchen sink demo (CTRL-F would trigger colorization), that should show how it works with different tokens.

This PR simply provides the low-level hooks for all kinds of highlighting scenarios—custom searches, high-contrast themes, ad-hoc token styling, etc.—and lets us iterate on the UX/API in follow-up discussions.

screen-capture.96.webm

@marinsokol5
Copy link
Contributor

Demo looks great, UI better than expected, especially with this partial token styling! Let's continue working on it and merge!

@mkslanc mkslanc force-pushed the color-change-selection branch from 4d18755 to a16f94b Compare July 8, 2025 11:43
@mkslanc mkslanc changed the title WIP: Text markers Feat: Implement text markers Jul 8, 2025
@mkslanc
Copy link
Contributor Author

mkslanc commented Jul 8, 2025

@marinsokol5 hey! I've finished the PR and embedded the demo into the kitchen sink. Quick question - do we need a separate option in the kitchen sink panel to enable/disable it, or is it okay to have it permanently turned on?

@mkslanc mkslanc marked this pull request as ready for review July 8, 2025 11:57
Copy link

codecov bot commented Jul 10, 2025

Codecov Report

Attention: Patch coverage is 96.73469% with 8 lines in your changes missing coverage. Please review.

Project coverage is 87.84%. Comparing base (020e0c4) to head (d158131).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/layer/text_markers.js 96.21% 5 Missing ⚠️
src/editor.js 33.33% 2 Missing ⚠️
src/layer/text_markers_test.js 99.03% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5835      +/-   ##
==========================================
+ Coverage   87.79%   87.84%   +0.04%     
==========================================
  Files         620      622       +2     
  Lines       46139    46383     +244     
  Branches     7564     7598      +34     
==========================================
+ Hits        40509    40745     +236     
- Misses       5630     5638       +8     
Flag Coverage Δ
unittests 87.84% <96.73%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@akoreman
Copy link
Contributor

Quick question - do we need a separate option in the kitchen sink panel to enable/disable it, or is it okay to have it permanently turned on?

My two cents, it might be confusing for users to see this styling in the kitchen sink as they might think it's out-of-the-box ace styling (in contrast to something they need to add themselves if they want it)

@mkslanc
Copy link
Contributor Author

mkslanc commented Jul 10, 2025

Quick question - do we need a separate option in the kitchen sink panel to enable/disable it, or is it okay to have it permanently turned on?

My two cents, it might be confusing for users to see this styling in the kitchen sink as they might think it's out-of-the-box ace styling (in contrast to something they need to add themselves if they want it)

I agree, but not sure, what would be the good way to bring it in demo

@akoreman
Copy link
Contributor

I agree, but not sure, what would be the good way to bring it in demo

Yeah I'm also not sure, could it make sense to not have it in the kitchen sink for now so that we can merge the PR and then add a demo for it later?

@mkslanc mkslanc force-pushed the color-change-selection branch from 3bf9b30 to d158131 Compare July 13, 2025 15:33
@mkslanc
Copy link
Contributor Author

mkslanc commented Jul 13, 2025

I agree, but not sure, what would be the good way to bring it in demo

Yeah I'm also not sure, could it make sense to not have it in the kitchen sink for now so that we can merge the PR and then add a demo for it later?

Removed it for now; btw: I almost finished working on PR based on that one, which would provide extension to show whitespaces in selection.

screen-capture.99.webm

@marinsokol5
Copy link
Contributor

Oh I liked the demo in the kitchen sink!
I do agree that red was a bit too much 😄 . I would be okay to set it as black text color, so we make the search selection in the kitchen sink accessible while demoing the feature at the same time.

Screenshot 2025-07-14 at 16 10 24

@mkslanc
Copy link
Contributor Author

mkslanc commented Jul 14, 2025

Oh I liked the demo in the kitchen sink! I do agree that red was a bit too much 😄 . I would be okay to set it as black text color, so we make the search selection in the kitchen sink accessible while demoing the feature at the same time.

Screenshot 2025-07-14 at 16 10 24

It would still take some extra effort - at least to detect light/dark themes and switch between black and white color. :)

@akoreman akoreman merged commit 085730f into ajaxorg:master Jul 15, 2025
3 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.

3 participants