Skip to content

W-2129587: removed wrapperStyle for LoadingSpinner to have full screen overlay#3696

Open
nayanavishwa wants to merge 1 commit intot/team404/sfp-on-pwafrom
nayanavishwa/W-21295874-overlay
Open

W-2129587: removed wrapperStyle for LoadingSpinner to have full screen overlay#3696
nayanavishwa wants to merge 1 commit intot/team404/sfp-on-pwafrom
nayanavishwa/W-21295874-overlay

Conversation

@nayanavishwa
Copy link
Collaborator

Description

Bug:

  • On checkout and cart page, when using express checkout, the overlay was not covering the entire page

Fix:

  • removed wrapperStyle from LoadingSpinner with height set to 100vh to make the overlay cover entire page.

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • (change1)

How to Test-Drive This PR

  • checkout this PR, run the app locally and verify the changes.

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@nayanavishwa nayanavishwa requested a review from a team as a code owner February 27, 2026 00:12
@nayanavishwa nayanavishwa requested review from alafemina and amittapalli and removed request for a team February 27, 2026 00:12
@cc-prodsec
Copy link
Collaborator

cc-prodsec commented Feb 27, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

return (
<CheckoutProvider>
{isDeletingUnavailableItem && <LoadingSpinner wrapperStyles={{height: '100vh'}} />}
{isDeletingUnavailableItem && <LoadingSpinner />}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure we should change this. I understand why you would identify the case though.

If this one existed before our branch, was it copied from yet another spinner case? I just wonder if there's a reason why it was done this way, which only fails for our payments case for some reason. After all 100vh is 100% of the viewport height.

Copy link
Collaborator Author

@nayanavishwa nayanavishwa Feb 27, 2026

Choose a reason for hiding this comment

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

Screen.Recording.2026-02-27.at.11.37.02.AM.mov

Please check the recording. The issue might be more at the LoadingSpinner component level (LoadingSpinner -> Box component)where the position has to be fixed. I understand that changing at the component level will require testing through out the application.

The other alternative fix is:
<LoadingSpinner wrapperStyles={{ position: 'fixed' }} />

Let me know if you agree with this change. I will go ahead and update the PR.

The only other deviation I found in our code was checkout/index.jsx has used LoadingSpinner twice. I changed it to use it only once at CheckoutContainer, but this change did not solve the overlay issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really know the answer. I suggest invite the PWA Kit team (or whoever owns the spinner common component) to weigh in on the best practice. If there's an issue with the spinner maybe they can take it on and we fix/close our own bug when they've fixed it. If it's really just the wrapperStyles they should be able to choose the right value for it, and then we can simply apply that to our owned uses.

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