Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(playground): add settings button and group IR #3559

Merged
merged 4 commits into from
Nov 10, 2022

Conversation

dhrjarun
Copy link
Contributor

@dhrjarun dhrjarun commented Nov 4, 2022

Summary

Demo

playground-demo.mov

Try to fix #2620

  • Group Rome IR and Prettier IR into IR
    Screenshot 2022-11-04 at 13 47 35
  • Add settings buttons
  • fix overflow in SettingsMenu
    Screenshot 2022-11-04 at 13 34 44

Test Plan

@dhrjarun dhrjarun requested a review from a team November 4, 2022 08:11
@netlify
Copy link

netlify bot commented Nov 4, 2022

Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 08c166d
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/636d0154d1a8100009a5d7f7

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Hey @dhrjarun

Thanks for this contribution. I'm currently having permission issues with Netifly, so I cannot approve the preview build. I'm looking into it.

From the video, it seems to me that it's easily possible to accidentally open and directly close the settings menu again. Is this a problem or was it just a misclick from you when recording the video? How well does your solution work with keyboard-only navigation?

@dhrjarun
Copy link
Contributor Author

dhrjarun commented Nov 8, 2022

Hi @MichaReiser In the video, the settings menu opened and closed directly cuz of reload. I haven't added any shortcut key to open settings menu.

Copy link
Contributor

@MichaReiser MichaReiser 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 awesome!

I've two small improvement requests:

  • Add the aria-pressed attribute and set it to true if the settings menu is open (and false otherwise)
  • Do you have an idea of how to style the button when the menu is open? (you can use the [aria-pressed="true"] selector

@dhrjarun
Copy link
Contributor Author

dhrjarun commented Nov 9, 2022

How about this?

Screenshot 2022-11-09 at 18 29 01

@MichaReiser
Copy link
Contributor

That could work

@dhrjarun
Copy link
Contributor Author

dhrjarun commented Nov 9, 2022

When I have just onClick without onKeyDown, the button if focused and enter is pressed it gets a click which is desirable behaviour.

<button
	aria-label="Open or Close Settings Menu"
        aria-pressed={isSettingsVisible}
	onClick={() => setSettingVisible(!isSettingsVisible)}
	className="text-base text-slate-700 font-medium hover:text-slate-800 group"
	>
</button>

But It cause this error lint/a11y/useKeyWithClickEvents

When I add onKeyDown implementation like this it doesn't work.

<button
	aria-label="Open or Close Settings Menu"
        aria-pressed={isSettingsVisible}
	onClick={() => setSettingVisible(!isSettingsVisible)}
	onKeyDown={(event: React.KeyboardEvent<HTMLButtonElement>) => {
		if (event.key === "Enter") {
			setSettingVisible(!isSettingsVisible);
		}
	}}
	className="text-base text-slate-700 font-medium hover:text-slate-800 group"
	>
</button>

If I add onKeyDown implementation like this every key press except Enter key makes the button click.

<button
	aria-label="Open or Close Settings Menu"
        aria-pressed={isSettingsVisible}
	onClick={() => setSettingVisible(!isSettingsVisible)}
	onKeyDown={(event: React.KeyboardEvent<HTMLButtonElement>) => {
		setSettingVisible(!isSettingsVisible);
	}}
	className="text-base text-slate-700 font-medium hover:text-slate-800 group"
	>
</button>

I don't know if there is any use case of this rule lint/a11y/useKeyWithClickEvents with react in here.

@dhrjarun
Copy link
Contributor Author

dhrjarun commented Nov 9, 2022

And here's with arrow

demo.mov

@MichaReiser
Copy link
Contributor

MichaReiser commented Nov 10, 2022

When I have just onClick without onKeyDown, the button if focused and enter is pressed it gets a click which is desirable behaviour.

You can ignore (suppress) the rule for now. It's an issue with the lint rule. See #3644

Looks awesome! I really like the toggle animation

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Looks good. You only have to make sure that all files are correctly formatted with rome (You should be able to run pnpm format if you have Rust installed or use the VS Code extension)

@dhrjarun dhrjarun requested a review from a team November 10, 2022 13:31
@MichaReiser MichaReiser added the A-Website Area: website and documentation label Nov 10, 2022
@MichaReiser MichaReiser merged commit 2e2e64f into rome:main Nov 10, 2022
jeysal added a commit to jeysal/rometools that referenced this pull request Nov 10, 2022
* upstream/main:
  fix(npm/js-api): Import type from @rometools/backend-jsonrpc (rome#3647)
  doc(npm/js-api): Add experimental warning to README
  fix(npm/js-api): Lazy load backend implementations (rome#3652)
  feat(rome_lsp): stop the daemon after the last client disconnects (rome#3643)
  fix(npm/js_api): Ensure JS API build contains `dist` folder (rome#3648)
  fix(rome_cli): ensures the service only connects to compatible versions (rome#3642)
  feat(playground): add settings button and group IR (rome#3559)
  release: v10.0.1 (rome#3649)
  fix(rome_js_analyze): False positives for interactive elements in `useKeyWithClickEvents` (rome#3644)
  fix(rome_js_semantic): support for object and array bindings when exporting (rome#3620)
  doc(editors): Clarify Rome discovery (rome#3639)
  fix(rome_js_analyze): improve the detection of `ReactDOM.render` calls in `noRenderReturnValue` (rome#3626)
  chore(npm): add license (rome#3629)
  Add rustdocs index
  Add environment
  Change rust docs to use Netlify
  fix(rome_js_analyze): useValidAnchor considering all possible expressions (rome#3599)
  fix(rome_js_formatter): Trailing comma inside import rome#3600 (rome#3624)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Website Area: website and documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 Tiling Views
3 participants