davidhu2000 / react-spinners

A collection of loading spinner components for react
https://www.davidhu.io/react-spinners
MIT License
3.05k stars 264 forks source link

GridLoader is not displayed in grid #386

Closed GuillaumeDesforges closed 2 years ago

GuillaumeDesforges commented 3 years ago

Describe the bug The GridLoader is not displayed in grid, but in one line.

To Reproduce Steps to reproduce the behavior:

  1. Use GridLoader in any react project.

Expected behavior GridLoader dots should be displayed in a 3x3 grid, not on one unique line.

Screenshots

image

Additional context Must be because of https://github.com/davidhu2000/react-spinners/issues/159

Bug was not there in version 0.9.0 (I'm using this version to fix the issue temporarily).

Possible fix Use css to display as block.

davidhu2000 commented 3 years ago

I'm not able to reproduce via the demo site. Can you show me what the css looks like? I wonder if the css is inheriting something from the parent

GuillaumeDesforges commented 3 years ago

Sorry, I'm not on this projet anymore, and don't have the code anymore :/

davidhu2000 commented 3 years ago

i'll just close this issue for now and we can reopen with other reports.

twschiller commented 3 years ago

@davidhu2000 Thanks for your work on the library!

We have the same issue with the GridLoader as was originally reported, and reverting to 0.9.0 fixes the problem. It impacts our use of GridLoader everywhere. We'll report back with anything we find that might be the cause

Could you re-open in the meantime?

https://github.com/pixiebrix/pixiebrix-extension/issues/755

davidhu2000 commented 2 years ago
import GridLoader from "react-spinners/GridLoader";

const DEFAULT_STYLE = {
  margin: "auto", // Center
  padding: "20px",
  display: "flex",
  justifyContent: "center"
};

export default function App() {
  return (
    <div style={DEFAULT_STYLE} data-testid="loader">
      <GridLoader />
    </div>
  );
}

@twschiller I tried to reproduce but couldn't. this is a copy/paste the Loader implementation.

I am able to reproduce by adding a with override like

<GridLoader css={{ width: 300 }} />

can ou check the css and see if that happens? did the default width get overwritten?

davidhu2000 commented 2 years ago

@twschiller can you try 0.13.0-alpha.5 and see if that resolves your issue?

fregante commented 2 years ago

I tried updating from 0.9.0 and the loader is completely broken/hidden:

Screen Shot 17

It should look like this:

Screen Shot 18

I don't know if I missed some required changes, I'm just calling it as <GridLoader /> but it's not appearing anywhere in my extension.

Same goes on in Storybook:

Screen Shot 19

Source: https://github.com/pixiebrix/pixiebrix-extension/blob/64ceada3a4539d44c6a4f50b407fa503d189772a/src/layout/Page.stories.tsx#L38-L44

davidhu2000 commented 2 years ago

turns out you can't do inline style with important tag :)

davidhu2000 commented 2 years ago

@twschiller @GuillaumeDesforges can you try on 0.13.0-beta.3? i think i fixed the issue.

davidhu2000 commented 2 years ago

Meant to tag @fregante :). Please give 0.13.0-beta.3 a try and see if it works

fregante commented 2 years ago

I think it's finally working, but I'm getting this TypeScript error in your latest beta.It's probably because I'm on React 17 and am using its types.

node_modules/react-spinners/helpers/props.d.ts:3:11 - error TS2430: Interface 'CommonProps' incorrectly extends interface 'ClassAttributes<HTMLSpanElement> & HTMLAttributes<HTMLSpanElement>'.
  Type 'CommonProps' is not assignable to type 'HTMLAttributes<HTMLSpanElement>'.
    Types of property 'css' are incompatible.
      Type 'CSSProperties' is not assignable to type 'InterpolationWithTheme<any>'.
        Type 'Properties<string | number, string & {}>' is not assignable to type 'ObjectInterpolation<undefined>'.
          Types of property 'columnCount' are incompatible.
            Type 'ColumnCount' is not assignable to type 'ColumnCountProperty | ColumnCountProperty[] | ("auto" | Globals)[]'.
              Type 'string & {}' is not assignable to type 'ColumnCountProperty | ColumnCountProperty[] | ("auto" | Globals)[]'.
                Type 'String' is missing the following properties from type '("auto" | Globals)[]': pop, push, join, reverse, and 21 more.

3 interface CommonProps extends DetailedHTMLProps<HTMLAttributes<HTMLSpanElement>, HTMLSpanElement> {
davidhu2000 commented 2 years ago

Hmm, that look more like @emotion error? Maybe I need to rename the css prop to styleOverride or something. See if that helps

davidhu2000 commented 2 years ago

seems related to https://github.com/emotion-js/emotion/issues/1800? altho I don't see emotion in your repo, can you try deleting node_modules and reinstall everything? thinking maybe the prior installation is causing some conflicts

twschiller commented 2 years ago

altho I don't see emotion in your repo, can you try deleting node_modules and reinstall everything? thinking maybe the prior installation is causing some conflicts

We include react-select in our project, which depends on emotion for styling:

pixiebrix/extension@1.6.5-alpha.1 /Users/tschiller/projects/pixiebrix-extension
└─┬ react-select@5.3.2
  └── @emotion/react@11.9.0

Will have to track down which emotion release the fix for https://github.com/emotion-js/emotion/issues/1800 would have been rolled into

davidhu2000 commented 2 years ago

Let me know what you find. Given the number of projects using emotion, I'm tempted to rename the prop to avoid conflicts. Just in case this package is used alongside emotion

davidhu2000 commented 2 years ago

decided that renaming the prop made sense. it was only called css to match emotion. and now that emotion is removed, css might just cause confusion. https://github.com/davidhu2000/react-spinners/pull/524

@twschiller want to give 0.13.0-beta.5 a shot and see if the type issue is resolved?

twschiller commented 2 years ago

@davidhu2000 Thanks! It's working but the grid loader animation is a bit jumpy (not smooth) compared to 0.9.0. Any ideas what might cause that? Will post a video later today

davidhu2000 commented 2 years ago

Yeah a video will be helpful. Given this is a total rewrite of the component, wouldn't be surprised if I missed a thing or two

davidhu2000 commented 2 years ago

so comparing the code before https://github.com/davidhu2000/react-spinners/blob/aee6124bcf6a603983d8eafbe334c97ad32d8bd2/src/GridLoader.tsx

vs current

https://github.com/davidhu2000/react-spinners/blob/main/src/GridLoader.tsx nothing is jumping out at me that could suggest the choppiness.

altho the animation is based on a random number, maybe sometimes it looks choppy and ok other times?

twschiller commented 2 years ago

Apologies for the delay, we were in crunch on a release the past couple weeks. Here's a Loom of the issue: https://www.loom.com/share/553454bd82fc471f8ecbd81b6a905b09

If I just throw it into Storybook, I don't see the jumpiness. So I think it has to do with re-renders/remounts. We might have to memoize the component or fix our render tree. (The jumpiness is probably on our side)

With the 0.13.0-beta.5 we're still having to do a CSS fix for the GridLoader not being displayed in a grid

If we don't do that, we're still seeing the bug from the original ticket: image

I double-checked I'm on the right version: image

davidhu2000 commented 2 years ago

actually i see that on my side in storybook as well. Will see what i can figure out

davidhu2000 commented 2 years ago

looks like adding display: 'inline-block' resolves the issue for me. @twschiller , Can you try 0.13.0 and see if it works? I moved the latest version out of beta.

Please let me know if you run into any more issues

twschiller commented 2 years ago

Can you try 0.13.0 and see if it works?

@davidhu2000 Thanks, looks like this version fixes the line break problem

patrikw1 commented 1 year ago

Turns out for me it was inherited white-space nowrap from bootstap's navbar brand

white-space: normal do it for me