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(Team Dashboard): Temporarily hide team stats block #362

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

HenerHoop
Copy link

Copy link

github-actions bot commented Nov 16, 2023

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 66.98 KB (+0.17% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.87 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 27.77 KB (0%)
packages/cxl-ui/pkg/dist-web/vendor.js 135.58 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js 243.35 KB (+0.05% 🔺)

@HenerHoop HenerHoop force-pushed the hener/feat/temporarily-hide-team-stats-block branch from 0fdfea6 to e41e987 Compare November 16, 2023 22:18
@HenerHoop HenerHoop requested a review from pawelkmpt November 16, 2023 22:21
Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

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

Commit message

- feat(Team Dashboard): Temporarily hide team stats block
+ feat(cxl-ui): cxl-dashboard-team-stats add hide progress, stats properties

I flipped the logic to make it a bit clearer. Instead of show, it's now hide.

IMO using
${this.showProgress ? html`` : nothing

is easier to read than
${!this.hideProgress ? html`` : nothing

but let it stay and not waste time on opinions ;)

@HenerHoop HenerHoop force-pushed the hener/feat/temporarily-hide-team-stats-block branch from e41e987 to d2fb82d Compare November 17, 2023 08:40
@HenerHoop
Copy link
Author

Commit message

- feat(Team Dashboard): Temporarily hide team stats block
+ feat(cxl-ui): cxl-dashboard-team-stats add hide progress, stats properties

I flipped the logic to make it a bit clearer. Instead of show, it's now hide.

IMO using ${this.showProgress ? html : nothing ``

is easier to read than ${!this.hideProgress ? html : nothing ``

but let it stay and not waste time on opinions ;)

I agree with you. From a developer's perspective, it might be a bit harder to read, but in terms of overall logic, it seems more logical to me that the element is visible in normal situations, and then we want to hide it in certain cases.

@pawelkmpt
Copy link

@HenerHoop conflicts appeared after merging #361. Please resolve them

@HenerHoop HenerHoop force-pushed the hener/feat/temporarily-hide-team-stats-block branch from d2fb82d to 9b23945 Compare November 17, 2023 09:24
@HenerHoop HenerHoop requested a review from pawelkmpt November 17, 2023 09:25
@pawelkmpt pawelkmpt merged commit 7b72d57 into master Nov 17, 2023
4 checks passed
@pawelkmpt pawelkmpt deleted the hener/feat/temporarily-hide-team-stats-block branch January 9, 2024 12:49
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.

2 participants