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

fix: query playground cursor [LIBS-177] #1380

Merged
merged 9 commits into from Apr 16, 2024

Conversation

KaiVandivier
Copy link
Contributor

@KaiVandivier KaiVandivier commented Apr 10, 2024

Implements LIBS-177

The issue may be OS/font dependent -- I've never been able to reproduce it on MacOS, but Mozafar can consistently (on what I think is Ubuntu, but haven't confirmed). The reporter of the issue in Jira was using Windows. I was able to reproduce the cursor issue on MacOS by using a non-monospace font, which seems to agree with other people's experience with brace/react-ace, but I didn't find a font-related fix that worked for Mozafar.

So now this PR switches out react-ace for react-codemirror to see if that fixes the issue

Uploaded here for testing: https://debug.dhis2.org/2.40dev/api/apps/query-playground/

Regarding scrolling, the behavior changed when switching to Codemirror due to different internal scrolling behavior. I figured out a minimal proof of concept for the desired styles and scrolling behavior, but it was complicated to get the dynamic/relative sizing to work with lots of nested elements with different heights and flex rules, so I ended up using a bit of a hardcode-ey way to fix the scrolling

Scrolling problem demo:

scrolling-problem-demo.mov

Minimal fix example:

minimal-example.mov

Applied fix:

fixed-scrolling.mov

Copy link
Contributor

@kabaros kabaros left a comment

Choose a reason for hiding this comment

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

I can confirm this fixes the problem in Ubuntu

Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

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

LGTM! Let's merge this as a chore and then publish from the example dir:

  1. Copy in id and any other necessary info from app hub app to d2.config.js
  2. Update version in package.json to be 1.0.1 (or maybe 1.1.0...). We don't use yarn version because we don't want a tag in this repo.
  3. Run yarn run d2-app-scripts publish to publish to the app hub. Maybe turn this into a package.json script (or maybe not)
  4. Write all of this in a README for the query playground so we remember it next time
  5. Push the updated config and package.json to github with another chore commit
  6. Consider moving this out of the monorepo some day...

Kai we can work on this together first thing tomorrow if you want!

@KaiVandivier KaiVandivier merged commit 5d58f2a into master Apr 16, 2024
14 checks passed
@KaiVandivier KaiVandivier deleted the libs-177-query-playground-cursor branch April 16, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants