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

feat: Add esquery selector textfield & highlighting of matched code #80

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

Conversation

pkerschbaum
Copy link
Contributor

@pkerschbaum pkerschbaum commented Feb 9, 2025

Prerequisites checklist

What is the purpose of this pull request?

MVP for esquery selector playground (#79): textfield to enter selectors, matching code get's highlighted:

image

Future ideas:

  • add matches count, tooltips for matches, or syntax highlighting of the selector itself like https://regexr.com/

What changes did you make? (Give an overview)

  • add reusable component TextField, styled similar to Button
  • add textfield for esquery selector in first (left) panel
  • add switch to nav bar to toggle the esquery selector textfield
    • is by default off --> people need to opt-into displaying the esquery selector textfield
  • add Codemirror extension + CSS to highlight text ranges via its "Decoration" feature
  • if the esquery toggle is enabled, and the esquery selector matches nodes
    • highlight text ranges in the editor
    • highlight nodes in the AST tree tool

Related Issues

Fixes #79

Is there anything you'd like reviewers to focus on?

Copy link

linux-foundation-easycla bot commented Feb 9, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Feb 9, 2025

Deploy Preview for eslint-code-explorer ready!

Name Link
🔨 Latest commit 4cd9966
🔍 Latest deploy log https://app.netlify.com/sites/eslint-code-explorer/deploys/67b785359ea6cf00085e6022
😎 Deploy Preview https://deploy-preview-80--eslint-code-explorer.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pkerschbaum pkerschbaum force-pushed the feat/highlight-esquery-matched-code branch 4 times, most recently from 20dc06d to 928859e Compare February 9, 2025 16:55
@pkerschbaum pkerschbaum force-pushed the feat/highlight-esquery-matched-code branch from 928859e to 9483a16 Compare February 9, 2025 16:55
@pkerschbaum pkerschbaum marked this pull request as ready for review February 9, 2025 17:04
@nzakas
Copy link
Member

nzakas commented Feb 10, 2025

Thanks for putting this together. Some notes on the UI:

  1. I don't think having a textbox span the entire width of the app is the right way to go. I was thinking it should be inside one of the panes.
  2. If we include ESQuery, it should work for all languages, not just JavaScript, so I don't think toggling in the language dropdown is the right location. Instead, what about adding a button that is next to the theme button in the toolbar?
  3. Highlighting the text in the editor is great. I think we also need to highlight the nodes in the tree from the start.

For some perspective, check out the typescript-eslint playground that has an ESQuery filter:
https://typescript-eslint.io/play/

@amareshsm
Copy link
Member

How about placing the ESQuery search option on the left pane, as shown in the image below?

image

@pkerschbaum
Copy link
Contributor Author

sounds good, will do!

@pkerschbaum
Copy link
Contributor Author

I implemented all suggestions:

In order to have 1 place to apply the match nodes via the esquery selector, I added a useAST hook where the JS/CSS/Markdown/JSON code-to-AST parsing happens now.

The esquery matched nodes need to be prop-drilled everywhere, and the accordion highlighting is duplicated. That's not ideal but I think in the future we could refactor the code anyways to get the accordion code generalized at a central place.

@pkerschbaum
Copy link
Contributor Author

@nzakas @amareshsm plz re-review :)

@nzakas
Copy link
Member

nzakas commented Feb 18, 2025

@pkerschbaum This is looking really nice! A few notes from playing around with it:

  • I think the "Show ESQuery Selector" toggle is unnecessary. Let's just leave the textbox there.
  • The focus color for the textbox should be one of the purple shades in light mode (right now it's black, which looks very stark).
  • It seems like it would be helpful to shade the textbox background reddish (check the available colors in our theme stylesheet) when there are no matches. Otherwise, it's not clear that nothing matches.
  • When I type "Program" in the JS view, the node is not highlighted on the right. (I checked all of the other languages and the root node is correctly highlighted, so not sure why JS is special.)
  • When I first opened the app and hit "Show ESQuery Selector", it crashed. I believe this is the same issue as Bug: New languages don't work #76, where something new is added to the state and a user who has already visited the app and has state stored in localStorage gets a crash. I think be removing the toggle we'll avoid this problem because we won't be changing what's stored in the state.

@amareshsm
Copy link
Member

Looks great! In addition to Nzaka's feedback, I’ve listed a couple of minor points below.

  1. When I type a program in the ESQuery Selector, the code gets highlighted, but the program node isn't highlighted in the AST preview.

image

  1. Let's add a placeholder text to the ESQuery Selector text field.

@pkerschbaum
Copy link
Contributor Author

thx for review!

  1. I removed the toggle.
  2. I fixed the issue that the root node of JS AST tree view would not get highlighted (forgot 1 place to put the logic - we really need to unify some logic ^^)
  3. That dark focus outline is currently aligned with how the focus looks like for all other buttons - to be consistent. Still, do you want me to change it to purple shade?
    image
  4. I added the red background if the selector does not match anything; as destructive of the available tailwind colors was too harsh, see screenshot...
    image ...so I added one of the codemirror-themes.css as nonmatchingEsquerySelector tailwind color:
    image
  5. I could not reproduce the crash - but I made the access onto the new state property more safe, hope that avoids it.

@pkerschbaum
Copy link
Contributor Author

Also added the placeholder

@nzakas @amareshsm plz re-review :)

@amareshsm
Copy link
Member

amareshsm commented Feb 18, 2025

That dark focus outline is currently aligned with how the focus looks like for all other buttons - to be consistent. Still, do you want me to change it to purple shade?

+1 with updating the color to one of the purple shades to align with the design we use across other ESLint sites. We’re using the color shade --color-primary-800 in other parts of the site.

we can this change in separate PR also.

@nzakas
Copy link
Member

nzakas commented Feb 19, 2025

@pkerschbaum no need to ask for reviews, we get the notifications. 😄 We just have a lot to get through every day so your patience is appreciated.

Ah, I hadn't even noticed that the focus outline was the same in other form controls. We can leave it as-is for now. No need to bundle unrelated changes in this PR.

Let's be sure to stick to the colors that are already in this file:
https://github.com/eslint/code-explorer/blob/main/src/codemirror-themes.css

These are all of our theme colors and we don't want to stray outside of that. I think one of the color-rose values would work for the textbox (though we may need a different color for dark mode).

Overall this is looking really good.

@pkerschbaum
Copy link
Contributor Author

I implemented the rose-red highlighting when the esquery selector has no matches.
I kept the black focus outline - I suggest to change that in a followup PR for all inputs together 👍
So, should be ready for re-review

@amareshsm
Copy link
Member

In JSON language, when I enter "STRING" in the ESQuery selector, it causes the UI to break.

@pkerschbaum
Copy link
Contributor Author

In JSON language, when I enter "STRING" in the ESQuery selector, it causes the UI to break.

damn, didn't see that... fixed with 4cd9966

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

This is looking so good!

There's one strange behavior I found: When the ESQuery selector has no matches, the background turns reddish but only if the cursor isn't over the textbox. If the cursor is over the textbox, the background turns a lighter color and the reddish color is lost.

I think the reddish color should stay regardless of mouse position. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: add functionality to test esquery selectors
3 participants