fix: issue #73 - Githubissues

topcoder-platform / micro-frontends-taas-admin-app

taas-admin-app for managing taas administrative activities

MIT License

0 stars 5 forks source link

fix: issue #73 #74

Closed yoution closed 3 years ago

yoution commented 3 years ago

@maxceem please review

yoution commented 3 years ago

@maxceem can we just disable the checkbox, and not show the tooltip? in another case， there are two rows, if first row is disabled by reason A, second row is disabled by B, how to show the tooltip? reason A + B?

maxceem commented 3 years ago

@yoution For rows you did it in a good way. 2 reasons are displayed, it's nice.

For the "select all" checkbox I still prefer to have a tooltip so it is clear for the user why it's disabled. It should not be hard to add a Tooltip to the checkbox, see how we add it inside rows https://github.com/topcoder-platform/micro-frontends-taas-admin-app/blob/dev/src/routes/WorkPeriods/components/PeriodItem/index.jsx#L142-L158

For this "select all" checkox, there would be always only one reason. No need to implement multiple reasons like inside rows.

yoution commented 3 years ago

ok, I will add two reasons

maxceem commented 3 years ago

ok, I will add two reasons

hmm, @yoution this is already done, no need to add two reasons, see. This already works automatically inside rows:

Only one thing is needed:

disable "select all" checkox
show tooltip for disabled select all checkbox
only ONE reason in the tooltip for "select all" checkox

Sorry if I confuse you, feel free to contact me on Slack if you like to have a quick discussion.