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

🐛 "Effort" with supplemental text info via tooltip #1846

Merged
merged 15 commits into from
May 10, 2024

Conversation

kpunwatk
Copy link
Contributor

@kpunwatk kpunwatk commented Apr 14, 2024

@kpunwatk
Copy link
Contributor Author

Hi @ibolton336 @sjd78 @mguetta1 Please review this PR, Thanks!

@mguetta1 mguetta1 changed the title Changed 'Effort' to 'Total Effort' on application inventory page 🐛 Changed 'Effort' to 'Total Effort' on application inventory page Apr 15, 2024
Copy link
Collaborator

@mguetta1 mguetta1 left a comment

Choose a reason for hiding this comment

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

LGTM

@mguetta1 mguetta1 added the cherry-pick/release-0.3 This PR should be cherry-picked to release-0.3 branch. label Apr 15, 2024
@sjd78 sjd78 added this to the v0.3.2 milestone Apr 15, 2024
@ibolton336
Copy link
Member

ibolton336 commented Apr 16, 2024

I'm not sure if "total effort" is working as expected:
image
image

Seems as though the total is not equal to the sum of all incidents effort. May be best to leave as effort until we sort this out with backend team.

Copy link
Member

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

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

See comment.

@mguetta1
Copy link
Collaborator

Hi @ibolton336
I don't see the problem. The screenshot you shared is "Affected applications" by Issue: File system - Java IO:
The total effort for the "jira" application is: 15 (probably there are more than 1 issue) and the side drawer shows the effort for the specific issue (8 incidents found in 3 files, the effort for the specific issue is 1)

Hope it helps.

@ibolton336
Copy link
Member

ibolton336 commented Apr 16, 2024

Hi @ibolton336 I don't see the problem. The screenshot you shared is "Affected applications" by Issue: File system - Java IO: The total effort for the "jira" application is: 15 (probably there are more than 1 issue) and the side drawer shows the effort for the specific issue (8 incidents found in 3 files, the effort for the specific issue is 1)

Hope it helps.

Screenshot 2024-04-16 at 8 37 18 AM

So "Effort" on the issues page is really an "effort weight" since it is not really the calculated value for any application? Seems like adding "total" just adds more confusion IMO since there is not a clear correlation between "effort" and "total effort". I think we could keep the name "effort" and just add a tooltip to clarify what it means in each of the 3 instances (total file calculated effort of all incidents for this file, total application effort, and effort weight) of the column, but @JustinXHale may have a better idea.

@ibolton336
Copy link
Member

Also, most screen sizes wont have the screen real estate to show "total effort" on the apps page leaving us with a truncated title for the effort column:

Screenshot 2024-04-16 at 8 43 47 AM

@mguetta1
Copy link
Collaborator

So "Effort" on the issues page is really an "effort weight" since it is not really the calculated value for any application?

Correct, effort for a single issue incident

@kpunwatk
Copy link
Contributor Author

So "Effort" on the issues page is really an "effort weight" since it is not really the calculated value for any application? Seems like adding "total" just adds more confusion IMO since there is not a clear correlation between "effort" and "total effort". I think we could keep the name "effort" and just add a tooltip to clarify what it means in each of the 3 instances (total file calculated effort of all incidents for this file, total application effort, and effort weight) of the column.

I agree. We could simply add a tooltip to avoid truncation. Additionally, I propose renaming 'Total effort' back to 'Effort' in the 'Affected Applications' section, What do you all think?

@ibolton336
Copy link
Member

So "Effort" on the issues page is really an "effort weight" since it is not really the calculated value for any application? Seems like adding "total" just adds more confusion IMO since there is not a clear correlation between "effort" and "total effort". I think we could keep the name "effort" and just add a tooltip to clarify what it means in each of the 3 instances (total file calculated effort of all incidents for this file, total application effort, and effort weight) of the column.

I agree. We could simply add a tooltip to avoid truncation. Additionally, I propose renaming 'Total effort' back to 'Effort' in the 'Affected Applications' section, What do you all think?

I think moving back to just "Effort" with supplemental text info via tooltip would be a good approach. It maintains consistency & would provide more clarity.

@kpunwatk
Copy link
Contributor Author

I think moving back to just "Effort" with supplemental text info via tooltip would be a good approach. It maintains consistency & would provide more clarity.

Thanks, @ibolton336 . I will make these modifications and update the PR

@kpunwatk kpunwatk force-pushed the mta-2556 branch 2 times, most recently from ba6f8f9 to d376fd4 Compare April 22, 2024 15:59
@kpunwatk kpunwatk changed the title 🐛 Changed 'Effort' to 'Total Effort' on application inventory page 🐛 "Effort" with supplemental text info via tooltip Apr 22, 2024
@kpunwatk
Copy link
Contributor Author

Hi @ibolton336 @sjd78 @mguetta1
For consistency moving back to just "Effort" with supplemental text info via tooltip,
Please review the PR, Thanks !
Screenshot from 2024-04-20 22-53-23
Screenshot from 2024-04-20 22-53-18
Screenshot from 2024-04-20 22-53-02

Signed-off-by: Karishma Punwatkar <[email protected]>
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.

Adding the tooltip and the externalized strings LGTM

@sjd78 sjd78 requested a review from ibolton336 April 23, 2024 16:06
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.

Some i18n keys need to be fixed up. Hopefully that'll also satisfy the e2e tests.

…1856)

After successful import re-enable useFetchApplications() hook for the
next 15 seconds.

Resolves: https://issues.redhat.com/browse/MTA-2451

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

@mguetta1 mguetta1 left a comment

Choose a reason for hiding this comment

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

I would like to test it, but npm install fails:

[mguetta@fedora tackle2-ui]$ npm install .
npm ERR! code ENETUNREACH
npm ERR! syscall connect
npm ERR! errno ENETUNREACH
npm ERR! request to https://registry.npmjs.org/axios/-/axios-1.6.8.tgz failed, reason: connect ENETUNREACH 2606:4700::6810:1922:443

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/mguetta/.npm/_logs/2024-05-01T11_49_10_124Z-debug-0.log

@sjd78
Copy link
Member

sjd78 commented May 1, 2024

I would like to test it, but npm install fails:

[mguetta@fedora tackle2-ui]$ npm install .
npm ERR! code ENETUNREACH
npm ERR! syscall connect
npm ERR! errno ENETUNREACH
npm ERR! request to https://registry.npmjs.org/axios/-/axios-1.6.8.tgz failed, reason: connect ENETUNREACH 2606:4700::6810:1922:443

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/mguetta/.npm/_logs/2024-05-01T11_49_10_124Z-debug-0.log

That's a strange one for sure. I haven't seen the ENETUNREACH error before. Plenty of other weird errors, just not that one. Right now, the versions node>=18.14.2 and npm@^9.5.0 are enforced in the root package.json. So you should be using an ok version.

Try this to really start up with a fresh node_modules:

> npm run clean:all
> npm clean-install --ignore-scripts
> num run start:dev

@mguetta1
Copy link
Collaborator

mguetta1 commented May 2, 2024

Thanks @sjd78
I run it from https://github.com/kpunwatk/tackle2-ui/tree/mta-2556 as it is, and get the same error:

[mguetta@fedora tackle2-ui]$ npm run clean:all

> @konveyor-ui/[email protected] clean:all
> npm run clean && rimraf ./node_modules ./**/node_modules/


> @konveyor-ui/[email protected] clean
> rimraf ./dist && npm run clean -ws --if-present


> @konveyor-ui/[email protected] clean
> rimraf ./dist


> @konveyor-ui/[email protected] clean
> rimraf ./dist


> @konveyor-ui/[email protected] clean
> rimraf ./dist

[mguetta@fedora tackle2-ui]$ npm clean-install --ignore-scripts
npm ERR! code ENETUNREACH
npm ERR! syscall connect
npm ERR! errno ENETUNREACH
npm ERR! request to https://registry.npmjs.org/axios/-/axios-1.6.8.tgz failed, reason: connect ENETUNREACH 2606:4700::6810:1b22:443

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/mguetta/.npm/_logs/2024-05-02T10_44_52_462Z-debug-0.log

@sjd78
Copy link
Member

sjd78 commented May 2, 2024

@mguetta1

npm ERR! request to https://registry.npmjs.org/axios/-/axios-1.6.8.tgz failed, reason: connect ENETUNREACH 2606:4700::6810:1b22:443

Maybe it is an IPv6 error on your end when it is trying to download the axios dep? The ENETUNREACH error looks to be something on the user side of things. Can you force things to go IPv4?

FWIW, I've also tinkered with running a local verdaccio via podman/docker as an npmjs registry proxy to reduce internet traffic. Maybe something like that could help if your internet connectivity gets wonky?

	modified:   client/public/locales/en/translation.json
	modified:   client/src/app/pages/applications/applications-table/applications-table.tsx
	modified:   client/src/app/pages/issues/affected-applications/affected-applications.tsx
	modified:   client/src/app/pages/issues/issues-table.tsx

	modified:   client/public/locales/en/translation.json
	modified:   client/src/app/pages/applications/applications-table/applications-table.tsx
	modified:   client/src/app/pages/issues/affected-applications/affected-applications.tsx
	modified:   client/src/app/pages/issues/issue-detail-drawer/issue-affected-files-table.tsx
	modified:   client/src/app/pages/issues/issues-table.tsx

	modified:   client/public/locales/en/translation.json
@sjd78 sjd78 removed the cherry-pick/release-0.3 This PR should be cherry-picked to release-0.3 branch. label May 3, 2024
@sjd78 sjd78 modified the milestones: v0.3.2, v0.5.0 May 3, 2024
Copy link

codecov bot commented May 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.22%. Comparing base (b654645) to head (afacca5).
Report is 136 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1846      +/-   ##
==========================================
+ Coverage   39.20%   41.22%   +2.02%     
==========================================
  Files         146      158      +12     
  Lines        4857     5189     +332     
  Branches     1164     1269     +105     
==========================================
+ Hits         1904     2139     +235     
- Misses       2939     3032      +93     
- Partials       14       18       +4     
Flag Coverage Δ
client 41.22% <ø> (+2.02%) ⬆️
server ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sjd78 sjd78 merged commit ba67f92 into konveyor:main May 10, 2024
10 checks passed
ibolton336 added a commit that referenced this pull request Jun 5, 2024
…atch displayed text (#1933)

Resolves: #1936 
UI Tests PR: 1124
Addresses mismatch between data column and displayed text for "Effort"
Column in affected applications table

PR #1846 intended to revert back to using effort
favoring tooltips to differentiate what is being displayed in fields
labeled effort. The Issues>Affected Application table effort column
heading text was updated, but the data-label used was still "Total
Effort".

Signed-off-by: Ian Bolton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants