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

feat: Table view framework and related facilities #3027

Merged
merged 113 commits into from
Aug 14, 2024
Merged

Conversation

traeok
Copy link
Member

@traeok traeok commented Jul 30, 2024

Proposed changes

  • Implement an extensible and customizable table view class (Table.View)
  • Add a builder facility for quickly building tables (TableBuilder)
  • Mediator facility for exposing tables to Zowe Explorer and other extenders (TableMediator)
  • Set up a panel view for showing tables in a fixed spot in Zowe Explorer
    • Expose a facility for updating the table in this view: TableViewProvider
  • Patch coverage for all mentioned facilities

Unrelated changes

  • Fix small code warning with zedc for zip_path being unused

Note that some slight adjustments to this framework may occur to fully support the tabular jobs view in a separate branch. This branch only contains the framework and data structures for making, exposing and rendering generic tables.

Dev. documentation on wiki

Release Notes

Milestone: vNext

Changelog:

  • Implements the ability to render tables in Zowe Explorer
  • Add table builder and mediator to facilitate customization and extensibility for table views

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds or improves functionality)
  • Breaking change (a change that would cause existing functionality to not work as expected)
  • Documentation (Markdown, README updates)
  • Other (please specify above in "Proposed changes" section)

Checklist

General

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • All PR dependencies have been merged and published (if applicable)
  • A GIF or screenshot is included in the PR for visual changes
  • The pre-publish command has been executed:
    • v2 and below: yarn workspace vscode-extension-for-zowe vscode:prepublish
    • v3: pnpm --filter vscode-extension-for-zowe vscode:prepublish

Code coverage

  • There is coverage for the code that I have added
  • I have added new test cases and they are passing
  • I have manually tested the changes

Deployment

  • I have added developer documentation (if applicable)
  • Documentation should be added to Zowe Docs
    • If you're an outside contributor, please post in the #zowe-doc Slack channel to coordinate documentation.
    • Otherwise, please check with the rest of the squad about any needed documentation before merging.
  • These changes may need ported to the appropriate branches (list here):

traeok added 30 commits June 4, 2024 16:32
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

First round of weviews comments, many more to come. 😋
Code is LGTM so far! 🥳

@zFernand0 zFernand0 dismissed their stale review August 5, 2024 19:02

The main reason for requesting changes was addressed by Trae's explanation on packages/zowe-explorer-api/src/vscode/ui/utils/TableBuilder.ts 😋

@zFernand0 zFernand0 self-requested a review August 5, 2024 19:02
@traeok traeok requested a review from zFernand0 August 6, 2024 13:24
zFernand0
zFernand0 previously approved these changes Aug 7, 2024
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

I get some minor changes when running the prepublish script.
but aside from that, and all the other comments (which where mostly dumb questions for my own understanding)...
I do believe this LGTMerge! 😋

Copy link
Member

Choose a reason for hiding this comment

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

No need to update the changelog in the zFTP VSCE package since this was the only change that happened. 😋

Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

Looks pretty good, thanks @traeok! Left a few comments

packages/zowe-explorer-api/CHANGELOG.md Show resolved Hide resolved
packages/zowe-explorer-api/src/vscode/ui/TableView.ts Outdated Show resolved Hide resolved
packages/zowe-explorer/src/webviews/src/utils.ts Outdated Show resolved Hide resolved
packages/zowe-explorer-api/package.json Outdated Show resolved Hide resolved
packages/zowe-explorer-api/src/vscode/ui/TableView.ts Outdated Show resolved Hide resolved
zedc/src/code/prepare.rs Show resolved Hide resolved
Copy link

sonarqubecloud bot commented Aug 9, 2024

@traeok traeok requested a review from t1m0thyj August 12, 2024 14:45
Copy link
Contributor

@JillieBeanSim JillieBeanSim left a comment

Choose a reason for hiding this comment

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

Thanks for this addition @traeok, I may be misunderstanding the PR but I don't see anything populated when selecting the Zowe Resources. I am also encountering issues with Job search in the tree for owner with my HLQ * * but can search by job id or when selecting the hyperlink after submitting. Will have to test to see if this is on next to and not just with PR.
testing on mac m1

@traeok
Copy link
Member Author

traeok commented Aug 13, 2024

Hi @JillieBeanSim, sorry for the confusion - this PR implements the table classes, logic and related facilities. To avoid a bunch of changed files for reviewers, I have been working on the jobs table view in a separate branch. We don't have a GitHub issue specific for the table framework, but I've attached it to the "jobs table" issue as it implements the generic components that will be used for it. I'll be creating a PR for the jobs table some time today/tomorrow 😁

As for the issues with job search, this is likely an existing issue with next as I have not adjusted the current logic for searching jobs in this PR.

@JillieBeanSim JillieBeanSim self-requested a review August 13, 2024 13:11
Copy link
Contributor

@JillieBeanSim JillieBeanSim left a comment

Choose a reason for hiding this comment

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

thanks @traeok for quick response, I found my issue with jobs search I mentioned so no worries for that, it was with my config. Code looks good and can address any issues with follow up PR. Thanks!

Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @traeok!

@@ -11,18 +11,22 @@
"preview": "vite preview",
"fresh-clone": "pnpm clean && rimraf node_modules",
"clean": "rimraf dist || true",
"package": "echo \"webviews: nothing to package.\"",
"package": "node -e \"fs.accessSync(path.join(__dirname, 'dist'))\" && echo \"webviews: nothing to package.\" || pnpm build",
Copy link
Member

Choose a reason for hiding this comment

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

No change required, but we may be able to use a relative path here since I think npm scripts already run in the context of the current directory 😋

Suggested change
"package": "node -e \"fs.accessSync(path.join(__dirname, 'dist'))\" && echo \"webviews: nothing to package.\" || pnpm build",
"package": "node -e \"fs.accessSync('dist')\" && echo \"webviews: nothing to package.\" || pnpm build",

@t1m0thyj t1m0thyj merged commit 4a52341 into next Aug 14, 2024
44 of 45 checks passed
@t1m0thyj t1m0thyj deleted the rnd/table-view branch August 14, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

Tabular display option for Jobs tree
4 participants