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

[WIP] ui: a more magical loading component #25013

Closed

Conversation

couchand
Copy link
Contributor

@couchand couchand commented Apr 23, 2018

A start on incorporating some of our learnings on how to structure the Loading component.

Some thoughts/questions:

  • TypeScript is annoying, I wish I didn't have to wrap it in a function
  • we could add error messages here, but I didn't because they're far from universal
  • this model I've got here breaks when the page relies on either: more than one CachedDataReducer (e.g. the Localities report) or some Reselect selector pulling in data from several places (e.g. anything using nodesSummary)
  • should it be a render prop instead of a render children?

Fixes #24011

@couchand couchand added the do-not-merge bors won't merge a PR with this label. label Apr 23, 2018
@couchand couchand requested a review from a team April 23, 2018 20:59
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@vilterp
Copy link
Contributor

vilterp commented Apr 23, 2018

:lgtm: I think this is the right place to start! A couple nits…

Writing down an idea from in-person about selectors: maybe our selectors should take cached data reducers and return cached data reducers… Or we could have a HOF which does this, like the map : (a -> b) -> m a -> m b function that monads have…


Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


pkg/ui/src/views/cluster/containers/nodeLogs/index.tsx, line 112 at r1 (raw file):

          <Loading
            data={ this.props.logs }
            className="loading-image loading-image__spinner-left"

can we get rid of these classes now? (same below)


pkg/ui/src/views/reports/containers/settings/index.tsx, line 14 at r1 (raw file):

// tslint:disable-next-line:variable-name
const Loading = buildLoading<SettingsResponseMessage>();

could also do class MessagesLoading extends Loading<SettingsResponseMessage> {} until we have JSX generics…


pkg/ui/src/views/shared/components/loading2/index.tsx, line 16 at r1 (raw file):

// * Loading will display a background image instead of the content if the
// * loading prop is true.
// *

nit: we use multiline comment syntax elsewhere (but I don't love it). Do you prefer this?


Comments from Reviewable

@couchand couchand closed this Nov 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge bors won't merge a PR with this label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants