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

@portaljs/components improvements/fixes #912

Closed
3 tasks done
luccasmmg opened this issue May 30, 2023 · 0 comments · Fixed by #933
Closed
3 tasks done

@portaljs/components improvements/fixes #912

luccasmmg opened this issue May 30, 2023 · 0 comments · Fixed by #933
Assignees

Comments

@luccasmmg
Copy link
Member

luccasmmg commented May 30, 2023

This is an issue to track improvements/fixes to @portaljs/components based on user feedback

Acceptance criteria

  • The components that fetch data should be able to fetch it even with CORs on Firefox ❌ 2023-06-06 WONTFIX. This is related to the firefox policy of sending a pre-flight request when the Range header is set. See notes below for more info
  • The loading indicator for the FlatUiTable should be of an appropriate size ✅ 2023-06-06 FIXED. See PR: @portaljs/components improvements #933
  • Loading indicators on all data components ✅ 2023-06-06 FIXED. Table and LineChart now have loading indicators.

Notes

CORS Issues on Firefox

Because of the way that Firefox handles CORs differently than Chrome, most of the components never render there, if you go to https://example.portaljs.org/@luccasmmg/test-data-repo-1 you will be shown this.
image
It never stops loading

Investigation notes

The

component loads data in the same way as the component (by using fetch). For some reason, fails due to a pre-flight request. After analyzing the code, the only difference is that the latter is sending a Range header.

Request that works:

GET /datasets/finance-vix/main/data/vix-daily.csv HTTP/2
Host: raw.githubusercontent.com
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Firefox/102.0
Accept: */*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
Referer: http://localhost:6006/
Origin: http://localhost:6006
Connection: keep-alive
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: cross-site
Pragma: no-cache
Cache-Control: no-cache
TE: trailers

Request that fails:

GET /datasets/finance-vix/main/data/vix-daily.csv undefined
Host: raw.githubusercontent.com
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Firefox/102.0
Accept: */*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
Referer: http://localhost:6006/
range: bytes=0-5132288
Origin: http://localhost:6006
Connection: keep-alive
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: cross-site

After removing the Range header, it works. I tried a few combination of other headers to make this work but it seems like the Range header always causes a pre-flight request on Firefox.

The solution is to simply use a corsProxy when using <FlatUiTable />, it seems like rn there's nothing else that can be done, except if we stop loading content partially, which will break a couple of pages due to overloads.

FlatuiTable loading indicator takes the whole width of the component

As shown in the image above, the loading indicator is too big for the FlatUiComponent

Components are missing loading indicators

The Table component, LineChart, Vega, VegaLite components dont have any loading indicators to them, they just pop up once the data is loaded, this will probably be very tricky to do on LineChart, Vega, VegaLite as we don't have a way to control whats going on inside them, but should be doable on the Table component.

@luccasmmg luccasmmg assigned luccasmmg and demenech and unassigned luccasmmg Jun 5, 2023
demenech added a commit that referenced this issue Jun 6, 2023
…dd that to <Table /> when using the URL parameter
demenech added a commit that referenced this issue Jun 6, 2023
@demenech demenech linked a pull request Jun 6, 2023 that will close this issue
luccasmmg pushed a commit that referenced this issue Jun 7, 2023
* [#912,@portaljs/components][s]: create LoadingSpinner component and add that to <Table /> when using the URL parameter

* [#912,@portaljs/components][m]: refactor LineChart and add more params

* [#912,@portaljs/components][m]: possibly fixes 'loading...' size on FlatUiTable on Firefox

* [@portaljs/components][xs]: add storybook-static to .gitignore

* Add changeset
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 a pull request may close this issue.

2 participants