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

Show more info in tooltip for members and IFS files #1153

Merged
merged 7 commits into from
Apr 25, 2023
Merged

Show more info in tooltip for members and IFS files #1153

merged 7 commits into from
Apr 25, 2023

Conversation

chrjorgensen
Copy link
Collaborator

@chrjorgensen chrjorgensen commented Mar 22, 2023 •
edited
Loading

Changes

This PR will enhance the tooltips for members and IFS files by adding

  • number of lines (for members)
  • size (for IFS files)
  • Change date
  • owner (for IFS files)

Requirements for IFS files tooltips

For tooltips to show up, both stat and sort OSS utilities must be installed! This can be done by the following command:

yum install coreutils-gnu

Checklist

  • have tested my change
  • eslint is not complaining
  • for feature PRs: PR only includes one feature enhancement.

@chrjorgensen chrjorgensen added the enhancement New feature or request label Mar 22, 2023
@chrjorgensen
Copy link
Collaborator Author

@sebjulliand I know this PR will conflict with your change adding sort by date or name. I will add this if you want me to - by sorting in the client instead of SQL and ls since this PR switches from ls to stat if available.

@worksofliam
Copy link
Contributor

@chrjorgensen Before I review this, would you mind fixing this conflict? It looks an awesome change.. seriously good work here

@worksofliam
Copy link
Contributor

Also, thank you making this work with and without stat - my system didn't have it!

worksofliam
worksofliam previously requested changes Mar 23, 2023
View reviewed changes
Copy link
Contributor

@worksofliam worksofliam left a comment

Choose a reason for hiding this comment

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

Looks good other than some minor bits. Please also merge the conflicts and we should be good to go for some testing.

src/views/ifsBrowser.js Outdated Show resolved Hide resolved
src/views/ifsBrowser.js Outdated Show resolved Hide resolved
src/views/ifsBrowser.js Outdated Show resolved Hide resolved
src/api/IBMiContent.ts Show resolved Hide resolved
@chrjorgensen
Copy link
Collaborator Author

@worksofliam

Also, thank you making this work with and without stat - my system didn't have it!

Yeah, it would be a breaking change if it was not optional. 😃

Comments resolved and conflict fixed - please test.

@chrjorgensen chrjorgensen marked this pull request as draft March 23, 2023 17:52
@chrjorgensen
Copy link
Collaborator Author

@worksofliam Please note that this change conflicts in functionality with the PR #1151 by @sebjulliand - so this should not be merged into master yet, since the sort functionality must be implemented here as well. I have converted this to a draft to avoid accidental merge...

@worksofliam worksofliam removed their assignment Mar 25, 2023
@worksofliam worksofliam dismissed their stale review March 25, 2023 18:48

Not ready for review yet

@worksofliam
Copy link
Contributor

@chrjorgensen You got it! I am looking forward to this feature a lot. Feel free to request a review from me when it is ready!

@chrjorgensen chrjorgensen self-assigned this Mar 26, 2023
@chrjorgensen chrjorgensen marked this pull request as ready for review April 21, 2023 22:36
@chrjorgensen chrjorgensen force-pushed the feature/more-info-in-tooltip branch from 41b994e to 90847b6 Compare April 21, 2023 22:41
@chrjorgensen
Copy link
Collaborator Author

@worksofliam @sebjulliand Please have a review of this PR, which has now had the master branch merged in and some additional fixes. The merging of the master branch was a bit cumbersome with the fine sorting options made by @sebjulliand. I have tested that the sorting options still work, but please review carefully anyway. 💣

@chrjorgensen
Copy link
Collaborator Author

@sebjulliand Can I persuade you to check this PR? The code had some conflicts with your fine sort options, and maybe I didn't merge it correct?

@sebjulliand
Copy link
Collaborator

I will for sure, no worries. I'll have a look today or tomorrow!

@chrjorgensen
Copy link
Collaborator Author

Thanks, much appreciated!

sebjulliand
sebjulliand approved these changes Apr 25, 2023
View reviewed changes
Copy link
Collaborator

@sebjulliand sebjulliand left a comment

Choose a reason for hiding this comment

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

Darn good job! This is a really sweet addition to the browsers 👍🏻
Sorting still work as expected, no worries about that.
And that epoch computation in SQL is so nice, too!

@sebjulliand sebjulliand merged commit ee76907 into codefori:master Apr 25, 2023
@chrjorgensen chrjorgensen deleted the feature/more-info-in-tooltip branch April 25, 2023 08:28
@chrjorgensen chrjorgensen mentioned this pull request Apr 25, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebjulliand sebjulliand sebjulliand approved these changes

@worksofliam worksofliam Awaiting requested review from worksofliam

Assignees

@sebjulliand sebjulliand

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chrjorgensen @worksofliam @sebjulliand