fix ui changes in helm chart view #2874

msivasubramaniaan · 2023-05-10T10:08:37Z

Signed-off-by: msivasubramaniaan msivasub@redhat.com

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

codecov · 2023-05-10T10:16:00Z

Codecov Report

Patch coverage: 70.76% and project coverage change: +2.72 🎉

Comparison is base (c1fef59) 33.77% compared to head (9e240db) 36.50%.

❗ Current head 9e240db differs from pull request most recent head fbc123c. Consider uploading reports for the commit fbc123c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2874      +/-   ##
==========================================
+ Coverage   33.77%   36.50%   +2.72%     
==========================================
  Files          55       54       -1     
  Lines        4059     3668     -391     
  Branches      768      711      -57     
==========================================
- Hits         1371     1339      -32     
+ Misses       2688     2329     -359

Impacted Files	Coverage Δ
src/odo.ts	`44.23% <0.00%> (+11.05%)`	⬆️
src/openshift/openshiftItem.ts	`24.69% <ø> (+5.82%)`	⬆️
src/webview/helm-chart/helmChartLoader.ts	`24.63% <42.85%> (+0.82%)`	⬆️
src/odo/command.ts	`21.95% <50.00%> (+8.09%)`	⬆️
src/openshift/component.ts	`20.19% <87.50%> (+4.88%)`	⬆️
src/oc.ts	`100.00% <100.00%> (+80.00%)`	⬆️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

mohitsuman · 2023-05-10T21:48:48Z

There is too much of gap between cards. We need to remove the grid layout and reduce the space between the cards.
Also it needs to be responsive, so that if we increase the zoom in, the cards should automatically layout itself in the next row.
Make the card layout same as we have in DefaultDevfile Registry card structure.

mohitsuman · 2023-05-10T21:50:04Z

I cannot see the cursor on the chart name. Once i start typing, then the words show up. The cursor should be highlighted once the focus is on the input box.

src/webview/helm-chart/app/cardItem.tsx

mohitsuman

Added some UX changes with card and modal view.

src/webview/helm-chart/app/cardItem.tsx

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

rgrunber

Definitely looks like a better use of space!

Before

After

Before

After

Are we sure about the red underline for "Description" & "Product version" ?
Also I noticed the "Install" button becomes Red when it can be clicked, while before it was more of a purple (standard light-theme button colour). Is this intentional ?

I would feel free to merge when ready though as this is a step in the right direction.

msivasubramaniaan · 2023-05-16T06:51:39Z

Hello @rgrunber,
Thanks for your comments.

Are we sure about the red underline for "Description" & "Product version" ? - Yes, @mohitsuman provides the requirement to underline the keys which we are showing in detailed panel.
Also I noticed the "Install" button becomes Red when it can be clicked, while before it was more of a purple (standard light-theme button colour). Is this intentional ? - Yes that intentional. Basically it was disabled and when your provides the required details and the button will be enabled with red color.

mohitsuman · 2023-05-16T10:39:46Z

@msivasubramaniaan Few UX changes:

It truncates the full name of the Helm Chart. The full name is `BkS400 Helm

The name is displayed incorrectly. The correct name is a10networks-a10tkc

Remove provided by from Top and add after Description. Make sure the name is formatted in Sentence Case (such as Red Hat, Community, Partner).
Label: Source
Text: Community/Red Hat/Partner

Let's remove the Red underline and make the label bold.

Increase a bit of padding from search bar and cards

Add more left margin for the install button.

mohitsuman

Suggested UX Changes

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

mohitsuman · 2023-05-21T20:18:16Z

@msivasubramaniaan Here are the new review comments.

Install Button is out of the modal view

The Helm Chart a10networks-a10tkc is still incorrect.
Provider section is missing. This should be after the Description tab.

mohitsuman · 2023-05-21T20:21:07Z

src/webview/helm-chart/app/cardItem.tsx

+                    <div style={{ width: '50%', minHeight: '5rem', maxHeight: '10rem' }}>
+                        {selectedVersion.description &&
+                            <div className={this.props.cardItemStyle.detailedDescription}>
+                                <Typography variant='body1' className={this.props.cardItemStyle.helmCardDetailItem}>Description</Typography>


Suggested change

<Typography variant='body1' className={this.props.cardItemStyle.helmCardDetailItem}>Description</Typography>

<Typography variant='body1' style = {{ margin: '30px', wordBreak: 'break-word'}}>Description</Typography>

Also the css applied here has margin: 0px, we need to remove that, so that the description text below has some line difference. I see the css applied is coming from global reference.

mohitsuman

Added the comments around UX.

mohitsuman · 2023-05-21T20:25:00Z

src/webview/helm-chart/app/cardItem.tsx

+                        }
+                        {selectedVersion.maintainers &&
+                            <div className={this.props.cardItemStyle.detailedDescription}>
+                                <Typography variant='body1' className={this.props.cardItemStyle.helmCardDetailItem}>Maintainers</Typography>


The css padding for the modal has to be looked into as the space below the Maintainers div is not getting loaded.

For reference see the image, the padding for the Maintainers div is not used.

mohitsuman · 2023-05-21T20:26:23Z

src/webview/helm-chart/app/cardItem.tsx

-                                    }
-                                </Typography>
+                        <div style={{ display: 'flex', flexDirection: 'row', width: '100%', position: 'fixed' }}>
+                            <div className={this.props.cardItemStyle.devPageTitle} style={{ width: '70%', gap: '2rem' }}>


Having width fixed to 70% is not useful if we need dynamic layout. For small screens, it will go out of modal view.

msivasubramaniaan · 2023-05-22T07:29:18Z

The Helm Chart a10networks-a10tkc is still incorrect. - What you meant a10networks-a10tkc is not the correct one. We are getting this name only on our response. Please refer the attachment

Provider section is missing. This should be after the Description tab. - The Provider changed to Source. And for attached screenshot there were no descriptions in response

mohitsuman · 2023-05-22T07:58:08Z

The API response gives the correct name, but the UI does not display it correctly. It should display a10networks-a10tkc but it skips the -.

I see you have added Source, why not just keep it as Provider ? It will be consistent to devconsole.

msivasubramaniaan · 2023-05-22T08:10:10Z

The API response gives the correct name, but the UI does not display it correctly. It should display a10networks-a10tkc but it skips the -.

In most of the cases dev console were removed '-' and displayed the names like Canvas Oamportal instead of canvas-oamportal. Don't know why they haven't done the same for a10networks-a10tkc. On our case I have removed the '-' for all names and displayed in the chart

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

mohitsuman

LGTM. For other enhancements around progress and notification, we will handle in a different PR.

fix ui changes in helm chart view

7831cfd

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

msivasubramaniaan requested a review from mohitsuman May 10, 2023 10:08

msivasubramaniaan self-assigned this May 10, 2023

removed bgcolor and added underline

fa41c61

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>

mohitsuman reviewed May 10, 2023

View reviewed changes

src/webview/helm-chart/app/cardItem.tsx Show resolved Hide resolved

mohitsuman reviewed May 10, 2023

View reviewed changes

src/webview/helm-chart/app/cardItem.tsx Show resolved Hide resolved

mohitsuman reviewed May 10, 2023

View reviewed changes