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(TMG-2609): Update web implementation for commenting entitlement #4029

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

sauravarora04
Copy link
Contributor

Description

Please include a summary of the change along with relevant motivation and context.

JIRA-1234

Checklist

  • Have you done any manual testing?
  • Does it have automated tests?
  • Do you need any other PRs merged before this (if so please list)?
  • Do you need to update the README/Runbook
  • Have you checked for accessibility?

Screenshots (if appropriate):

Include screenshots if needed.

@sauravarora04 sauravarora04 requested a review from a team as a code owner January 8, 2025 14:53
@sauravarora04 sauravarora04 force-pushed the feat/TMG-2609-commenting-entitlement-3 branch from bb5fa1c to fef9c66 Compare January 8, 2025 16:14
@Extracreative Extracreative force-pushed the feat/TMG-2609-commenting-entitlement-3 branch from fef9c66 to 6d774dd Compare January 10, 2025 10:40
@sauravarora04 sauravarora04 force-pushed the feat/TMG-2609-commenting-entitlement-3 branch from 6d774dd to e0292b5 Compare January 10, 2025 14:00
if (!isEnabled && !isCommentEnabled) {
content = <DisabledComments />;
} else if (hasCommentingEntitlement) {
content = (

Choose a reason for hiding this comment

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

Why not just use conditional rendering? This pattern is... unusual

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of this pattern was to wrap different condition results within a user state wrapper.

Copy link
Contributor

Choose a reason for hiding this comment

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

What are the benefits of using the user state wrapper versus conditional rendering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the user-state component is dealing with all user states (like hasAccess, isLoggedin, etc.). I've just tried to re-use the existing pattern.

@sauravarora04 sauravarora04 force-pushed the feat/TMG-2609-commenting-entitlement-3 branch from 871f42a to 4c26b28 Compare January 15, 2025 12:13
@sauravarora04 sauravarora04 changed the title feat(TMG-000): testing pipeline error feat(TMG-2609): testing pipeline error Jan 16, 2025
@sauravarora04 sauravarora04 changed the title feat(TMG-2609): testing pipeline error feat(TMG-2609): Update web implementation for commenting entitlement Jan 16, 2025
@@ -110,7 +110,7 @@ jobs:
- run:
name: Run Web Tests
command: |
lerna run test:web --stream --since -- -- --ci --bail --coverage
lerna run test:web --since -- -- --ci --coverage --verbose --runInBand --logHeapUsage

Choose a reason for hiding this comment

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

So runInBand will probably help with memory usage, but it should also mean the tests run slower. Was this a required change?

Copy link

@lukejsikuade lukejsikuade left a comment

Choose a reason for hiding this comment

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

One comment, nothing major

@sauravarora04 sauravarora04 force-pushed the feat/TMG-2609-commenting-entitlement-3 branch 2 times, most recently from 8028854 to a92a80c Compare January 22, 2025 15:04
@sauravarora04 sauravarora04 force-pushed the feat/TMG-2609-commenting-entitlement-3 branch from 855317e to 7753887 Compare January 27, 2025 23:44
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.

4 participants