feat: 🎸 Add ResolvedQuery, LoadingEmptyState, and Tooltip #80

ibolton336 · 2021-10-14T00:30:56Z

This adds lib-ui candidates from the forklift-ui repo.

What: Closes #

Additional issues:

This adds lib-ui candidates from the forklift-ui repo

codecov · 2021-10-14T00:33:27Z

Codecov Report

Merging #80 (bba2ded) into main (126abe8) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main       #80   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           19        19           
  Branches         5         5           
=========================================
  Hits            19        19

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 126abe8...bba2ded. Read the comment docs.

konveyor-preview-bot · 2021-10-14T00:33:52Z

🚀 Deployed Storybook Preview: http://konveyor-lib-ui-pr-80-preview.surge.sh ✨

mturley

@ibolton336 this is great, just a couple of thoughts here (I know I'm nitpicking my own code, but I have meant to revisit these things before moving these to lib-ui anyway).

mturley · 2021-10-14T13:15:26Z

src/components/ConditionalTooltip.tsx

+  children: React.ReactElement;
+}
+
+const ConditionalTooltip: React.FunctionComponent<IConditionalTooltipProps> = ({


I'm still not 100% certain this component is necessary, I've been considering getting rid of it in forklift-ui. You can disable a tooltip by passing trigger="manual" and isVisible="false", but that doesn't feel clean either. I guess we can add this component but I might try to contribute a simpler isDisabled prop to the PF tooltip.

+1 Will remove for now.

mturley · 2021-10-14T13:18:13Z

src/components/ResolvedQuery/ResolvedQueries.tsx

+import { KubeClientError } from '../../modules/kube-client/types';
+import LoadingEmptyState from '../LoadingEmptyState';
+
+export enum QuerySpinnerMode {


I have been meaning to go and replace most of our string enums with simple string unions in forklift. I think it makes for cleaner TypeScript code, and I didn't realize you could do it when I wrote this. Let's change this to:

export type QuerySpinnerMode = 'inline' | 'emptyState' | 'none';

and then everywhere we are using spinnerMode={QuerySpinnerMode.Inline} we can just use spinnerMode="inline" etc, and we don't have to import the type.

mturley · 2021-10-14T13:24:33Z

src/components/ResolvedQuery/ResolvedQueries.tsx

+}: IResolvedQueriesProps) => {
+  const status = getAggregateQueryStatus(results);
+  const erroredResults = results.filter((result) => result.isError);
+  const filteredErrorTitles = errorTitles.filter((_title, index) => results[index].isError);


I have honestly been on the fence about whether having results and errorTitles as arrays was a good idea. If you're using ResolvedQueries to wait for several queries, especially if some of those are conditional, you have to make sure the errorTitles array is always the same length as the results array and they all match up. It's easy to screw up and not something TS can catch for us, and can lead to the wrong error message being displayed.

Maybe it's not worth holding up this PR, but I would love to reconsider these props, we could maybe have results be an array of objects like:

export interface IResolvedQueriesProps { results: { result: UnknownResult; errorTitle: string }[]; // ... }

That would enforce with TS that every query result gets an errorTitle, and then we wouldn't have to filter the errorTitles to match, we could just loop over the erroredResults and use the errorTitle from each. This might cause usage to look messier though, as it'll require a couple extra lines of code to pass in an array of objects like this.

I'm open to your thoughts on this @ibolton336 @seanforyou23 @gildub, I would be fine with merging this as-is and messing with a breaking change in forklift, or making this change now and potentially making a breaking revert or some other approach later. Open to other ideas as well.

mturley · 2021-10-14T13:25:50Z

src/components/ResolvedQuery/ResolvedQuery.tsx

+
+export const ResolvedQuery: React.FunctionComponent<IResolvedQueryProps> = ({
+  result,
+  errorTitle,


The result and errorTitle props could remain separate here though, because the above problem doesn't apply when there is just one of each. Then we'd pass them in as one object to ResolvedQueries below.

ibolton336 · 2021-10-14T18:25:08Z

src/components/ResolvedQuery/ResolvedQueries.tsx

+  forceLoadingState = false,
+  children = null,
+}: IResolvedQueriesProps) => {
+  const status = getAggregateQueryStatus(resultsMap.map((r) => r.result));


@mturley What do you think of this approach? Not sure if resultsMap is the best name for this list of objects.

The implementation looks great. I agree about the name, but I'm not sure I know the best solution.
I think maybe we still call the prop results, and call the type ResultsWithErrorTitles? TS will help if anyone tries to pass the wrong thing.

I'm still curious to see what it will look like to actually pass these in. a handful of extra lines in app code is worth avoiding the issue though I think. Thanks for this!

ibolton336 · 2021-10-14T18:30:25Z

src/components/ResolvedQuery/ResolvedQueries.tsx

+      ) : status === 'error' ? (
+        <AlertGroup aria-live="assertive">
+          {erroredResults.map((resultsMap, index) => {
+            const { result, errorTitle } = resultsMap;


Broke out the values from the map def here. Please let me know your thoughts.

ibolton336 · 2021-10-14T21:29:51Z

src/components/ResolvedQuery/ResolvedQuery.tsx

+import { ResolvedQueries, IResolvedQueriesProps } from './ResolvedQueries';
+
+export interface IResolvedQueryProps
+  extends Omit<IResolvedQueriesProps, 'results' | 'errorTitles'> {


Changed the resultsWithErrorTitles prop to optional as I was seeing some type errors when trying to do:

<ResolvedQuery result={plansQuery} errorTitle={"Could not load plans"} errorsInline={false} >

Typescript was wanting a resultsWithErrorTitles prop I think because I we are extending the ResolvedQueries prop types here? @mturley

Oh, we just need to change this:

extends Omit<IResolvedQueriesProps, 'results' | 'errorTitles'> {

to this:

extends Omit<IResolvedQueriesProps, 'resultsWithErrorTitles'> {

with that change the error should go away, I don't think we should make that prop optional.

Ahh ok thank you!

@mturley Just updated.

mturley

Awesome. Thanks! I opened an issue for one of us to follow up and create docs examples for these when we get a chance. #81

konveyor-release-bot · 2021-10-15T17:48:53Z

🎉 This PR is included in version 5.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: 🎸 Add ResolvedQuery, LoadingEmptyState, and Tooltip

355e259

This adds lib-ui candidates from the forklift-ui repo

ibolton336 requested a review from a team October 14, 2021 00:30

mturley reviewed Oct 14, 2021

View reviewed changes

Remove conditional tooltip & update results array to map

d561854

ibolton336 commented Oct 14, 2021

View reviewed changes

Change name from result(s)Map nto result(s)WithErrorTitle(s)

bba2ded

ibolton336 force-pushed the add-resolved-queries branch from e823b2d to bba2ded Compare October 15, 2021 00:30

mturley mentioned this pull request Oct 15, 2021

Add Stories/examples for ResolvedQuery, LoadingEmptyState #81

Open

mturley approved these changes Oct 15, 2021

View reviewed changes

mturley merged commit ed50b7e into migtools:main Oct 15, 2021

konveyor-release-bot added the released label Oct 15, 2021

ibolton336 deleted the add-resolved-queries branch October 15, 2021 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 🎸 Add ResolvedQuery, LoadingEmptyState, and Tooltip #80

feat: 🎸 Add ResolvedQuery, LoadingEmptyState, and Tooltip #80

ibolton336 commented Oct 14, 2021 •

edited

Loading

codecov bot commented Oct 14, 2021 •

edited

Loading

konveyor-preview-bot commented Oct 14, 2021

mturley left a comment

mturley Oct 14, 2021

ibolton336 Oct 14, 2021

mturley Oct 14, 2021

mturley Oct 14, 2021

mturley Oct 14, 2021

ibolton336 Oct 14, 2021

mturley Oct 14, 2021

ibolton336 Oct 14, 2021

ibolton336 Oct 14, 2021

mturley Oct 14, 2021

ibolton336 Oct 15, 2021

ibolton336 Oct 15, 2021

mturley left a comment

konveyor-release-bot commented Oct 15, 2021

feat: 🎸 Add ResolvedQuery, LoadingEmptyState, and Tooltip #80

feat: 🎸 Add ResolvedQuery, LoadingEmptyState, and Tooltip #80

Conversation

ibolton336 commented Oct 14, 2021 • edited Loading

codecov bot commented Oct 14, 2021 • edited Loading

Codecov Report

konveyor-preview-bot commented Oct 14, 2021

mturley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

konveyor-release-bot commented Oct 15, 2021

ibolton336 commented Oct 14, 2021 •

edited

Loading

codecov bot commented Oct 14, 2021 •

edited

Loading