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

Upgrade Chakra UI/React #1114

Merged
merged 55 commits into from
Mar 7, 2024
Merged

Upgrade Chakra UI/React #1114

merged 55 commits into from
Mar 7, 2024

Conversation

microbit-matt-hillsdon
Copy link
Collaborator

@microbit-matt-hillsdon microbit-matt-hillsdon commented Mar 6, 2023

  • Review alongside theme package PR (private)
  • Merge theme package and update in workflow
  • See if we can fix the ResizeObserve bug which is bad for local dev (resize sim)
  • Merge this PR

CI will likely fail hard because of the theme. I'll look at that next.
@github-actions
Copy link

github-actions bot commented Mar 6, 2023

Preview build will be at
https://review-python-editor-v3.microbit.org/chakra-upgrade/

There are issues with the LSP client init and the Sanity content
queries.
@microbit-matt-hillsdon microbit-matt-hillsdon marked this pull request as draft March 6, 2023 20:07
src/index.tsx Outdated Show resolved Hide resolved
@microbit-matt-hillsdon
Copy link
Collaborator Author

Based on e2e failures it looks like toast handling has changed, and perhaps not for the better in terms of accessibility. The underlying reach alert component should be good though. Needs careful review.

@microbit-matt-hillsdon
Copy link
Collaborator Author

microbit-matt-hillsdon commented Mar 10, 2023

There's a weird issue with About/Language dialogs (perhaps others) they leave their modal overlay behind when closed.

This only seems to affect the dialogs that always render themselves even when closed, not the ones that use our common dialog code.

Fixed by upgrading framer-motion.

I think the framer-motion upgrade fixes the dialogs.
@microbit-matt-hillsdon
Copy link
Collaborator Author

There's a new focus indicator in the search box.

@microbit-matt-hillsdon
Copy link
Collaborator Author

A bunch of simulator controls are unexpectedly enabled before running a program.

@microbit-matt-hillsdon
Copy link
Collaborator Author

A bunch of simulator controls are unexpectedly enabled before running a program.

This and similar issues fixed by 3375749

@microbit-matt-hillsdon
Copy link
Collaborator Author

microbit-matt-hillsdon commented Mar 10, 2023

DOM structure of toast is now an aria-live polite region.

Previously in the same position we had role=alert and a duplicate visually hidden live region that was aria-live polite.
So perhaps it was quite confusing before as there were two live regions (one via role) though I don't recall this being a practical issue. role=alert would have mapped to aria-live=assertive though so this might be a backwards step for error messages and a step forward for notifications?

I'll fix the e2e tests to match for now, as we need to find out if there are other issues.

New structure:

<div class="chakra-portal">
  <ul role="region" aria-live="polite" id="chakra-toast-manager-top"
    style="position: fixed; z-index: 5500; pointer-events: none; display: flex; flex-direction: column; margin: 0px auto; top: env(safe-area-inset-top, 0px); right: env(safe-area-inset-right, 0px); left: env(safe-area-inset-left, 0px);">
    <li class="chakra-toast"
      style="display: flex; flex-direction: column; align-items: center; opacity: 1; transform: none;">
      <div role="status" aria-atomic="true" class="chakra-toast__inner css-dixmdy">
        <div id="toast-1" class="chakra-alert css-13ta61m"><span class="chakra-alert__icon css-14ogjxt"><svg
              viewBox="0 0 24 24" focusable="false" class="chakra-icon css-qk6lof">
              <path fill="currentColor"
                d="M11.983,0a12.206,12.206,0,0,0-8.51,3.653A11.8,11.8,0,0,0,0,12.207,11.779,11.779,0,0,0,11.8,24h.214A12.111,12.111,0,0,0,24,11.791h0A11.766,11.766,0,0,0,11.983,0ZM10.5,16.542a1.476,1.476,0,0,1,1.449-1.53h.027a1.527,1.527,0,0,1,1.523,1.47,1.475,1.475,0,0,1-1.449,1.53h-.027A1.529,1.529,0,0,1,10.5,16.542ZM11,12.5v-6a1,1,0,0,1,2,0v6a1,1,0,1,1-2,0Z">
              </path>
            </svg></span>
          <div class="css-njbp03">
            <div id="toast-1-title" class="chakra-alert__title css-tidvy5">Cannot load file</div>
            <div id="toast-1-description" class="chakra-alert__desc css-161kwbg">This version of the Python Editor
              doesn't currently support adding .mpy files.</div>
          </div><button type="button" aria-label="Close" class="css-1pq15d"><svg viewBox="0 0 24 24" focusable="false"
              class="chakra-icon css-onkibi" aria-hidden="true">
              <path fill="currentColor"
                d="M.439,21.44a1.5,1.5,0,0,0,2.122,2.121L11.823,14.3a.25.25,0,0,1,.354,0l9.262,9.263a1.5,1.5,0,1,0,2.122-2.121L14.3,12.177a.25.25,0,0,1,0-.354l9.263-9.262A1.5,1.5,0,0,0,21.439.44L12.177,9.7a.25.25,0,0,1-.354,0L2.561.44A1.5,1.5,0,0,0,.439,2.561L9.7,11.823a.25.25,0,0,1,0,.354Z">
              </path>
            </svg></button>
        </div>
      </div>
    </li>
  </ul>
  <!-- Other regions for other toast positions here -->
</div>

Ah, just spotted there's a role=status. That still maps to polite.

open.test.ts passes except an issue with drag and drop
leaving CI to run the others
@microbit-matt-hillsdon
Copy link
Collaborator Author

Still 8 e2e failures. So far, apart from toast DOM structure, they've been async issues shown up by new timing, rather than anything broken.

src/index.tsx Outdated Show resolved Hide resolved
@microbit-matt-hillsdon
Copy link
Collaborator Author

I've released 3.0.18 with the various pending changes so that we can let this change sit on /v/beta for a while once merged.

@microbit-matt-hillsdon
Copy link
Collaborator Author

There's a significant visual difference in the About dialog which panicked me, but it turns out to be just due to the over-long version number on the review branch build.

@microbit-matt-hillsdon
Copy link
Collaborator Author

microbit-matt-hillsdon commented May 20, 2023

Visual changes:

  • Modal dialog version positions have changed very slightly. This seems fine.

  • Chakra selects have a slightly smaller arrow/different spacing around the arrow. No big deal.

  • There's a subtle difference in the gap between the gutter and the text in CodeMirror (sidebar code blocks and main editor). It looks worse so we should track it down.

    • That's this change: codemirror/view@a2d7f91 - there's no rationale so hopefully just aesthetic and we can override.
  • Focus ring behaviour has changed to be more typical - you only get it if you keyboard navigated to the control in the first place.

  • Modal focus management has regressed - if you create a new file via the Projects tab the focus isn't returned to the button. This seems to be systemic and is an unacceptable regression. Do we need to do something differently?

Since the above there's a new issue where focus is moved to the next button along. Needs more investigation, but a workaround is to use [email protected] (maybe newer, this was just known working from micro:bit classroom). Reproduces in a trivial new Chakra UI project so I'll raise a bug before long.

  • Docs or better approach for npm run theme which is now necessary to compile the code.

@microbit-matt-hillsdon microbit-matt-hillsdon marked this pull request as ready for review March 5, 2024 11:34
Copy link
Contributor

@microbit-grace microbit-grace left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@microbit-robert microbit-robert left a comment

Choose a reason for hiding this comment

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

LGTM.

@microbit-matt-hillsdon microbit-matt-hillsdon merged commit 365f6aa into main Mar 7, 2024
1 check passed
@microbit-matt-hillsdon microbit-matt-hillsdon deleted the chakra-upgrade branch March 7, 2024 09:33
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