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

GridLoader is not displayed in grid #386

Closed
GuillaumeDesforges opened this issue Mar 4, 2021 · 24 comments · Fixed by #513
Closed

GridLoader is not displayed in grid #386

GuillaumeDesforges opened this issue Mar 4, 2021 · 24 comments · Fixed by #513
Labels
Projects

Comments

@GuillaumeDesforges
Copy link

GuillaumeDesforges commented Mar 4, 2021

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 #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
Copy link
Owner

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
Copy link
Author

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

@davidhu2000
Copy link
Owner

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

@twschiller
Copy link

twschiller commented Jul 11, 2021

@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?

pixiebrix/pixiebrix-extension#755

@davidhu2000 davidhu2000 reopened this Jul 12, 2021
@davidhu2000
Copy link
Owner

davidhu2000 commented May 22, 2022

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
Copy link
Owner

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

@fregante
Copy link

fregante commented May 23, 2022

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 davidhu2000 reopened this May 26, 2022
@davidhu2000
Copy link
Owner

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

@davidhu2000
Copy link
Owner

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

@davidhu2000
Copy link
Owner

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

@fregante
Copy link

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
Copy link
Owner

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

@davidhu2000
Copy link
Owner

davidhu2000 commented Jun 5, 2022

seems related to emotion-js/emotion#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

@davidhu2000 davidhu2000 added this to In progress in 0.13 Jun 6, 2022
@twschiller
Copy link

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 emotion-js/emotion#1800 would have been rolled into

@davidhu2000
Copy link
Owner

davidhu2000 commented Jun 6, 2022

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
Copy link
Owner

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. #524

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

@twschiller
Copy link

@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
Copy link
Owner

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
Copy link
Owner

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
Copy link

twschiller commented Jun 17, 2022

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
Copy link
Owner

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

@davidhu2000
Copy link
Owner

davidhu2000 commented Jun 25, 2022

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

@davidhu2000 davidhu2000 moved this from In progress to Done in 0.13 Jun 25, 2022
@twschiller
Copy link

Can you try 0.13.0 and see if it works?

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

@patrikw1
Copy link

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

white-space: normal do it for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
0.13
Done
Development

Successfully merging a pull request may close this issue.

5 participants