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.

Sign up for GitHub

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

Jump to bottom

Convert Providers screen to Console page #33

Merged
merged 1 commit into from
Nov 20, 2022
Merged

Convert Providers screen to Console page #33

merged 1 commit into from
Nov 20, 2022

Conversation

rszwajko
Copy link
Contributor

@rszwajko rszwajko commented Aug 31, 2022 •
edited
Loading

Code changes:

  1. use StandardPage list component
  2. entity list is produced by merging results from k8s API and
    exsting Forklift inventory REST API.
  3. actions in the table (kebab actions) are provided via extension
    point 'console.action/provider'
  4. use existing actions for Add/Edit/Delete/Select Network

Functional changes:

  1. display all providers in one table
  2. use only 'Ready' condition to describe the state of the provider
  3. disable rich content in Network/Storage columns until the design for
    the details page is known

Several PR were extracted from this PR. Most important are #69 and #64.

@rszwajko rszwajko marked this pull request as draft August 31, 2022 14:11
@yaacov yaacov added the enhancement Categorizes issue or PR as related to a new feature. label Aug 31, 2022
@yaacov yaacov linked an issue Aug 31, 2022 that may be closed by this pull request
Convert forklift-ui pages to console pages #25
Closed
@rszwajko
Copy link
Contributor Author

rszwajko commented Sep 6, 2022 •
edited
Loading

Screenshots

image

image

image

image

image

image

image

image

image

@rszwajko rszwajko requested review from yaacov and sjd78 September 6, 2022 12:25
@rszwajko
Copy link
Contributor Author

rszwajko commented Sep 12, 2022 •
edited
Loading

Manage columns visibility and order

Provided functionalities:

  1. toggle column visibility (no restrictions on number/type of columns)
  2. re-order columns using drag and drop

Reference-Url: https://github.com/openshift/console/blob/master/frontend/public/components/modals/column-management-modal.tsx
Reference-Url: https://www.patternfly.org/v4/components/table/react-demos#column-management-with-draggable

@rszwajko rszwajko mentioned this pull request Sep 19, 2022
Merged
yaacov
yaacov reviewed Sep 20, 2022
View reviewed changes
src/Providers/components/ManageColumnsToolbar.tsx Outdated
defaultColumns,
}: ManagedColumnsProps) => {
const { t } = useTranslation();
const [editedColumns, setEditedColumns] = useState(columns);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should save the preferred columns on localStorage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add this in follow up.

@yaacov
Copy link
Member

yaacov commented Sep 20, 2022

@rszwajko @sjd78 hi,
I want to merge it, but don't want to create an unusable plugin (e.g. plugin with two different providers lists)
what do you think about creating a "next" branch and have the new lists/details pages there ?

@yaacov
Copy link
Member

yaacov commented Sep 28, 2022

@rszwajko can you add a signature so DCO will be happy ?

@codecov
Copy link

codecov bot commented Oct 4, 2022 •
edited
Loading

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (c99dbcb) compared to base (a2b6992).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@            Coverage Diff            @@
##              main       #33   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            8         8           
=========================================
  Hits             8         8

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@rszwajko
Copy link
Contributor Author

@rszwajko conflicts ?

The rebased version remains on a separate branch - https://github.com/rszwajko/forklift-console-plugin/tree/providersV3
Unfortunately, it has build problems - it seems that the extension type console.tab/horizontalNav is not recognized.

@rszwajko rszwajko mentioned this pull request Oct 28, 2022
Merged
@rszwajko
Copy link
Contributor Author

Forced pushed to rebase on top of PR #64 that was extracted from this PR.
The previous branch with commits aligned chronologically is available as branch providersV2

@rszwajko rszwajko force-pushed the providers branch 3 times, most recently from 4ee1bbc to 4950d37 Compare November 4, 2022 12:35
@rszwajko rszwajko mentioned this pull request Nov 4, 2022
Closed
@rszwajko
Copy link
Contributor Author

rszwajko commented Nov 4, 2022

Force pushed to extract details page to separate PR - see #69

@rszwajko rszwajko force-pushed the providers branch 3 times, most recently from e4d5a14 to f8731b0 Compare November 9, 2022 11:35
@rszwajko rszwajko marked this pull request as ready for review November 9, 2022 11:39
yaacov
yaacov reviewed Nov 9, 2022
View reviewed changes
src/utils/fetch.ts Outdated Show resolved Hide resolved
sjd78
sjd78 suggested changes Nov 10, 2022
View reviewed changes
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I learned a lot reviewing this one! Thanks!

In general, I know including import type XYZ from ''; or import { type XYZ, ABC } from ''; isn't required, but I've found it easier to read stuff when it is included. Maybe a linting rule can be added... ;-)

Nothing major to point out on my side. I think maybe we can trend towards a bit more straight forward data transforms in general go forward. The goal would be easy to read to verify data flow in headspace instead of having to rely on a debugger.

console-extensions.ts Outdated Show resolved Hide resolved
plugin.json Outdated Show resolved Hide resolved
src/extensions/ProvidersWrapper.tsx Outdated Show resolved Hide resolved
console-extensions.ts Outdated Show resolved Hide resolved
src/utils/fetch.ts Outdated Show resolved Hide resolved
src/Providers/ProvidersPage.tsx Outdated Show resolved Hide resolved
src/Providers/ProviderRow.tsx Outdated Show resolved Hide resolved
src/Providers/ProviderRow.tsx Outdated Show resolved Hide resolved
src/Providers/ProviderRow.tsx Outdated Show resolved Hide resolved
src/Providers/ProviderRow.tsx Outdated Show resolved Hide resolved
@yaacov yaacov mentioned this pull request Nov 16, 2022
Merged
@rszwajko rszwajko force-pushed the providers branch 2 times, most recently from 7f441b3 to c1b409d Compare November 16, 2022 18:21
sjd78
sjd78 reviewed Nov 16, 2022
View reviewed changes
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks great. I just have the few style comments to followup on.

src/modules/Providers/ProviderRow.tsx Outdated Show resolved Hide resolved
src/modules/Providers/data.ts Show resolved Hide resolved
@rszwajko rszwajko force-pushed the providers branch 3 times, most recently from 6f1a19c to 6fa0b5a Compare November 18, 2022 17:10
@rszwajko
Convert Providers screen to Console page
01c2968 
Code changes:
1. use StandardPage list component
2. entity list is produced by merging results from k8s API and
   exsting Forklift inventory REST API.
3. actions in the table (kebab actions) are provided via extension
   point 'console.action/provider'
4. use existing actions for Add/Edit/Delete/Select Network

Functional changes:
1. display all providers in one table
2. use only 'Ready' condition to describe the state of the provider
3. disable rich content in Network/Storage columns until the design for
   the details page is known

Signed-off-by: Radoslaw Szwajkowski <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rszwajko rszwajko requested review from sjd78 and yaacov November 18, 2022 17:30
sjd78
sjd78 reviewed Nov 18, 2022
View reviewed changes
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code LGTM, but when I tried to run in (crc target with Red Hat MTV 2.3.3 installed, yarn start and yarn start:console:oauth) a bunch of things went wrong.

The biggest is that no i18n text is showing up:
Screenshot from 2022-11-18 15-12-54

There is also an error no model for "Provider", but I'm not sure if it is related. Error details:

resourcePath: no model for "Provider" [react_devtools_backend.js:4026:25](moz-extension://52dc58e9-0fb5-4f21-b1bb-87e6c7a939b5/build/react_devtools_backend.js)
    overrideMethod react_devtools_backend.js:4026
    g main-chunk-8319c9271b351f114a18.min.js:1
    v main-chunk-8319c9271b351f114a18.min.js:1
    na vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    Hs vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    wc vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    Cc vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    _c vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    pc vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    Vi vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    unstable_runWithPriority vendors~main-chunk-5a9b3755878e2066e765.min.js:173817
    Hi vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    Vi vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    qi vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    sc vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    Aa vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    unsubscribe useBaseQuery.js:62
    NotifyManager notifyManager.js:62
    notifyFn notifyManager.js:10
    flush notifyManager.js:77
    flush notifyManager.js:76
    batchedUpdates$1 React
    flush notifyManager.js:75
    (Async: promise callback)
    scheduleMicrotask utils.js:322
    flush notifyManager.js:74
    batch notifyManager.js:30
    dispatch query.js:392
    setData query.js:85
    onSuccess query.js:336
    resolve retryer.js:58
    (Async: promise callback)
    run retryer.js:116
    Retryer retryer.js:156
    fetch query.js:332
    executeFetch queryObserver.js:199
    onSubscribe queryObserver.js:40
    subscribe subscribable.js:16
    useBaseQuery useBaseQuery.js:60
    jc vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    unstable_runWithPriority vendors~main-chunk-5a9b3755878e2066e765.min.js:173817
    Hi vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    Ec vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    pc vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    Vi vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    unstable_runWithPriority vendors~main-chunk-5a9b3755878e2066e765.min.js:173817
    Hi vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    Vi vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    N vendors~main-chunk-5a9b3755878e2066e765.min.js:173817
    onmessage vendors~main-chunk-5a9b3755878e2066e765.min.js:173817
    (Async: EventHandlerNonNull)
    <anonymous> vendors~main-chunk-5a9b3755878e2066e765.min.js:173817
    r runtime~main-bundle-4bd849060ba298736393.min.js:1
    <anonymous> vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    r runtime~main-bundle-4bd849060ba298736393.min.js:1
    <anonymous> vendors~main-chunk-5a9b3755878e2066e765.min.js:173504
    r runtime~main-bundle-4bd849060ba298736393.min.js:1
    <anonymous> vendors~main-chunk-5a9b3755878e2066e765.min.js:7053
    r runtime~main-bundle-4bd849060ba298736393.min.js:1
    <anonymous> main-chunk-8319c9271b351f114a18.min.js:1
    r runtime~main-bundle-4bd849060ba298736393.min.js:1
    <anonymous> main-chunk-8319c9271b351f114a18.min.js:1
    r runtime~main-bundle-4bd849060ba298736393.min.js:1
    d runtime~main-bundle-4bd849060ba298736393.min.js:1
    c runtime~main-bundle-4bd849060ba298736393.min.js:1
    <anonymous> vendors~main-chunk-5a9b3755878e2066e765.min.js:1

@yaacov
Copy link
Member

yaacov commented Nov 20, 2022

@sjd78 @rszwajko hi, the missing translation is because of #83 where I updated the i18next library version, I'm reverting the library bump in #84, so the strings should be visible after deleting the empty translations and re-running yarn i18n.

@yaacov yaacov merged commit 21d3f62 into kubev2v:main Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjd78 sjd78 sjd78 requested changes

@yaacov yaacov Awaiting requested review from yaacov

Assignees
No one assigned
Labels
enhancement Categorizes issue or PR as related to a new feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convert forklift-ui pages to console pages
3 participants
@rszwajko @yaacov @sjd78