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

Overlay look-and-feel polish #9230

Merged
merged 2 commits into from
Nov 29, 2023
Merged

Overlay look-and-feel polish #9230

merged 2 commits into from
Nov 29, 2023

Conversation

FredKSchott
Copy link
Member

Changes

Resolves a collection of feedback items from the bug bash:

  • Being able to close astro and settings panels to Esc
  • Multiple plugins visible open at once
  • z-index issue with audit message and window
  • add "No islands found" alert for the xray plugin
  • Config option for minimize by default?
  • Revisit dismiss/collapse behavior

The two major items tackled in this PR are:

  1. Toolbar UI/UX Polish -- Removed the previous explicit dismiss/collapse behavior and UI. The toolbar is now hidden by default, appears when you hover on/near it, and disappears soon afterwards if you move your mouse away. Escape key & clicking outside the toolbar are now possible. Shrank the toolbar to be less obtrusive.

  2. Improved Active Plugin Management -- There is now a single plugin active at any one time. Switching plugins now always closes a previously open plugin. This loses the "settings + another plugin open at the same time" nice-to-have story, but the simplicity gain is well worth it IMO.

Demo

Note: Screen Hero doesn't display keyboard input. There's a moment where the plugin closes and then the toolbar hides. This is because I pressed the escape key twice.

Astro.Dev.Overlay.Demo.mp4

Reviewing

This PR includes #9220. I'll rebase to remove from this PR as soon as that merges into next.

There was no obvious way to separate this PR, since the UI changes also impacted the behavior changes. I did my best and turned this into two different commits with two different focuses (first style, then behavior) but I would still recommend reviewing all in one go.

@Princesseuh I'd actually like to give you in person walkthrough as well, if you're available tomorrow. Similar to the logging work, I took a lot of discretion with look/feel here and want to merge that back together with the behaviors that you care about, if possible.

Testing

  • manually tested
  • audited for a11y, keyboard navigation

Docs

  • No docs changes needed afaik.
  • No APIs have changed.

Copy link

changeset-bot bot commented Nov 29, 2023

🦋 Changeset detected

Latest commit: 9d6640b

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Nov 29, 2023
Base automatically changed from feat/overlay-polish to next November 29, 2023 12:52
@matthewp
Copy link
Contributor

I'm bothered by the number of tabs you have open in your browser, but aside from that this lgtm.

@doodlemarks
Copy link
Contributor

Would love to help on the design side of things when it makes sense! I wasn't part of the bug bash or follow-up but seems like quite a few things changed.

Quick item I would adjust is the name "Menu" I would alter to "Home". Technically they are all menus so I found that to be odd.

@FredKSchott
Copy link
Member Author

Rebased! I walked through this PR this morning with @Princesseuh as well and got her verbal +1/LGTM with a couple of change requests. Merging now so that this PR isn't blocking future incremental improvements.

For @Princesseuh -- I did go ahead and remove two things that we discussed this morning from this PR, to re-tackle in the future:

  • the logic handling overlay transition animations: the desire to force display = 'none' on all hidden plugin canvases made it difficult to implement pretty transitions for plugin windows in a way that worked for first-party and third-party. I couldn't get it working across all variations. Not against adding this back (and I know you put a lot of time into this!) but I couldn't think of an easy way to do it so opted to remove. FWIW the instant no-transition has really grown on me!
  • advanced init handling: I didn't logic to handle another piece of complexity that I realized we'd need, where some plugins actually do want to re-run on view transition page transition (ex: re-scan for x-ray mode) but others wouldn't (ex: a dev designed init() to only run once). Ultimately I just moved to an explicit "init just runs once" model, and will create a separate follow-up issue to add back more advanced handling to explicitly solve for all of these edge cases (before or after launch).

@FredKSchott FredKSchott merged commit 60cfa49 into next Nov 29, 2023
13 checks passed
@FredKSchott FredKSchott deleted the fks/overlay-polish-2 branch November 29, 2023 22:18
@astrobot-houston astrobot-houston mentioned this pull request Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants