Updated user post screen UI #2046
Conversation
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
WalkthroughThe updates span various components, enhancing the user interface and functionality. Key changes include the addition of the Changes
Assessment against linked issues
Poem
Tip Early access features
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
src/screens/LoginPage/LoginPage.tsx (1)
Line range hint
85-88: ReplaceRegExpconstructor with regular expression literals.Using regular expression literals can improve performance and reduce complexity.
- lowercaseCharRegExp: new RegExp('[a-z]'), - uppercaseCharRegExp: new RegExp('[A-Z]'), - numericalValueRegExp: new RegExp('\\d'), - specialCharRegExp: new RegExp('[!@#$%^&*()_+{}\\[\\]:;<>,.?~\\\\/-]'), + lowercaseCharRegExp: /[a-z]/, + uppercaseCharRegExp: /[A-Z]/, + numericalValueRegExp: /\d/, + specialCharRegExp: /[!@#$%^&*()_+{}\\[\\]:;<>,.?~\\\\/-]/,Also applies to: 205-205
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (7)
- package.json (1 hunks)
- src/GraphQl/Mutations/OrganizationMutations.ts (1 hunks)
- src/components/OrgPostCard/OrgPostCard.tsx (1 hunks)
- src/components/UserPortal/PostCard/PostCard.module.css (2 hunks)
- src/components/UserPortal/PostCard/PostCard.tsx (2 hunks)
- src/screens/LoginPage/LoginPage.tsx (1 hunks)
- src/screens/UserPortal/Posts/Posts.tsx (8 hunks)
Files skipped from review due to trivial changes (2)
- package.json
- src/components/UserPortal/PostCard/PostCard.module.css
Additional context used
Biome
src/screens/UserPortal/Posts/Posts.tsx
[error] 178-178: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 307-307: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
src/components/OrgPostCard/OrgPostCard.tsx
[error] 221-225: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 323-323: Avoid the words "image", "picture", or "photo" in img element alt text. (lint/a11y/noRedundantAlt)
Screen readers announce img elements as "images", so it is not necessary to redeclare this in alternative text.
[error] 336-336: Avoid the words "image", "picture", or "photo" in img element alt text. (lint/a11y/noRedundantAlt)
Screen readers announce img elements as "images", so it is not necessary to redeclare this in alternative text.
[error] 353-362: Interactive elements should not be assigned non-interactive roles. (lint/a11y/noInteractiveElementToNoninteractiveRole)
WAI-ARIA roles should not be used to convert an interactive element to a non-interactive element.
Wrap your interactive element in awith the desired role or put the content inside your interactive element.
Unsafe fix: Remove the role attribute.
[error] 353-362: Enforce that elements with ARIA roles must use a valid, non-abstract ARIA role. (lint/a11y/useValidAriaRole)
Check WAI-ARIA for valid roles or provide options accordingly.
Unsafe fix: Remove the invalid role attribute.
Check the list of all valid role attributes.
[error] 388-393: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 396-401: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 404-409: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 412-416: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 522-522: Avoid the words "image", "picture", or "photo" in img element alt text. (lint/a11y/noRedundantAlt)
Screen readers announce img elements as "images", so it is not necessary to redeclare this in alternative text.
[error] 557-557: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 569-572: Provide a track for captions when using audio or video elements. (lint/a11y/useMediaCaption)
Captions support users with hearing-impairments. They should be a transcription or translation of the dialogue, sound effects, musical cues, and other relevant audio information.
[error] 353-362: Provide an explicit type prop for the button element. (lint/a11y/useButtonType)
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
[error] 367-371: Provide an explicit type prop for the button element. (lint/a11y/useButtonType)
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
[error] 374-378: Provide an explicit type prop for the button element. (lint/a11y/useButtonType)
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
[error] 524-528: Provide an explicit type prop for the button element. (lint/a11y/useButtonType)
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
[error] 573-577: Provide an explicit type prop for the button element. (lint/a11y/useButtonType)
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or resetsrc/screens/LoginPage/LoginPage.tsx
[error] 160-160: void is confusing inside a union type. (lint/suspicious/noConfusingVoidType)
Unsafe fix: Use undefined instead.
[error] 350-350: Provide a text alternative through the alt, aria-label or aria-labelledby attribute (lint/a11y/useAltText)
Meaningful alternative text on elements helps users relying on screen readers to understand content's purpose within a page.
[error] 361-361: Provide a text alternative through the alt, aria-label or aria-labelledby attribute (lint/a11y/useAltText)
Meaningful alternative text on elements helps users relying on screen readers to understand content's purpose within a page.
[error] 85-85: Use a regular expression literal instead of the RegExp constructor. (lint/complexity/useRegexLiterals)
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
[error] 86-86: Use a regular expression literal instead of the RegExp constructor. (lint/complexity/useRegexLiterals)
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
[error] 87-87: Use a regular expression literal instead of the RegExp constructor. (lint/complexity/useRegexLiterals)
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
[error] 88-88: Use a regular expression literal instead of the RegExp constructor. (lint/complexity/useRegexLiterals)
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
[error] 205-205: Use a regular expression literal instead of the RegExp constructor. (lint/complexity/useRegexLiterals)
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
Additional comments not posted (9)
src/screens/UserPortal/Posts/Posts.tsx (6)
5-5: The updated imports reflect changes in GraphQL query names and the addition of the carousel component for UI enhancement. This aligns with the PR objectives to update the user portal's post UI.
12-12: Introduction of new interfaces for handling advertisement list items enhances type safety and code readability, which is crucial for maintaining the new features related to the advertisement display.
28-45: The responsive settings for the carousel component are well-defined, providing a clear breakdown of behavior across different device sizes. This is a key part of improving the UI's responsiveness.
47-54: Definition of new types for advertisement handling is well done, ensuring that the data structure is clear and consistent, which will facilitate easier maintenance and feature extension.
335-340: The use of the carousel to display pinned posts is a significant UI improvement. It makes the interface more interactive and visually appealing, aligning with the modern design standards specified in the Figma design.
344-346: Displaying promoted posts in a dedicated section is a good use of space and resources, enhancing user engagement with important content.src/components/UserPortal/PostCard/PostCard.tsx (1)
277-277: The application of the new.postImageclass to the<Card.Img>component is a direct reflection of the CSS changes aimed at improving the visual presentation of posts. Ensure that the specified styles inPostCard.module.cssare correctly implemented to achieve the desired layout and aesthetics.src/components/OrgPostCard/OrgPostCard.tsx (1)
53-53: Including the title in thetogglePostPinmutation call is a necessary change to support the updated GraphQL mutation. This ensures that the title is available when toggling the pin status of a post, which can be useful for logging or conditional rendering in the UI.src/screens/LoginPage/LoginPage.tsx (1)
136-136: Ensure the use of optional chaining is consistent and safe.This code safely accesses nested properties which might be undefined. Good use of optional chaining to prevent runtime errors.
| mutation TogglePostPin($id: ID!, $title: String) { | ||
| togglePostPin(id: $id, title: $title) { |
There was a problem hiding this comment.
Tip
Codebase Verification
The inspection of the relevant lines indicates that:
- In
src/components/OrgPostCard/OrgPostCard.tsx, thetitleparameter is included in thetogglemutation call. - In
src/components/OrgPostCard/OrgPostCard.test.tsx, thetitleparameter is not included in the test's mutation call.
To avoid runtime errors, the test file should also include the title parameter. Here are the specific locations that need attention:
src/components/OrgPostCard/OrgPostCard.test.tsx: Ensure thetitleparameter is included in the mutation call.
Analysis chain
The addition of the title parameter to the TOGGLE_PINNED_POST mutation enhances the functionality by allowing the title of the post to be included when toggling its pinned status. Ensure that all calls to this mutation in the frontend now include this new parameter to avoid runtime errors.
The search results provided context around the instances of TOGGLE_PINNED_POST, but they still do not clearly show whether the title parameter is being included in the mutation calls. To definitively verify this, we need to inspect the relevant lines more closely.
The next step is to extract the specific lines of code where the mutation is called and check if the title parameter is included.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `TOGGLE_PINNED_POST` include the `title` parameter.
# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type graphql $'TOGGLE_PINNED_POST'
Length of output: 70
Script:
#!/bin/bash
# Description: Verify all function calls to `TOGGLE_PINNED_POST` include the `title` parameter.
# Test: Search for the function usage in JavaScript and TypeScript files. Expect: Only occurrences of the new signature.
rg 'TOGGLE_PINNED_POST' --type js --type ts
Length of output: 497
Script:
#!/bin/bash
# Description: Verify all function calls to `TOGGLE_PINNED_POST` include the `title` parameter.
# Inspect the relevant files for the correct usage of `TOGGLE_PINNED_POST` with the `title` parameter
rg --multiline --multiline-dotall 'TOGGLE_PINNED_POST' src/components/OrgPostCard/OrgPostCard.test.tsx src/components/OrgPostCard/OrgPostCard.tsx src/GraphQl/Mutations/mutations.ts
Length of output: 548
Script:
#!/bin/bash
# Description: Verify all function calls to `TOGGLE_PINNED_POST` include the `title` parameter.
# Extract more lines around each match to provide context
rg --multiline --multiline-dotall -C 5 'TOGGLE_PINNED_POST' src/components/OrgPostCard/OrgPostCard.test.tsx src/components/OrgPostCard/OrgPostCard.tsx src/GraphQl/Mutations/mutations.ts
Length of output: 4239
Script:
#!/bin/bash
# Description: Verify all function calls to `TOGGLE_PINNED_POST` include the `title` parameter.
# Extract specific lines around the mutation calls to check for the inclusion of the `title` parameter
rg --multiline --multiline-dotall -A 10 'useMutation\(TOGGLE_PINNED_POST\)' src/components/OrgPostCard/OrgPostCard.tsx
rg --multiline --multiline-dotall -A 10 'query: TOGGLE_PINNED_POST' src/components/OrgPostCard/OrgPostCard.test.tsx
Length of output: 738
| if (promotedPostsData && promotedPostsData.organizations) { | ||
| const ads: Ad[] = | ||
| promotedPostsData.organizations[0].advertisements?.edges.map( | ||
| (edge) => edge.node, | ||
| ); | ||
|
|
||
| setAdContent(ads); |
There was a problem hiding this comment.
The use of optional chaining here can prevent potential runtime errors when promotedPostsData.organizations might be undefined. This change is suggested by static analysis and should be incorporated to enhance code robustness.
- const ads: Ad[] = promotedPostsData.organizations[0].advertisements?.edges.map((edge) => edge.node);
+ const ads: Ad[] = promotedPostsData?.organizations[0]?.advertisements?.edges.map((edge) => edge.node) || [];Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (promotedPostsData && promotedPostsData.organizations) { | |
| const ads: Ad[] = | |
| promotedPostsData.organizations[0].advertisements?.edges.map( | |
| (edge) => edge.node, | |
| ); | |
| setAdContent(ads); | |
| if (promotedPostsData && promotedPostsData.organizations) { | |
| const ads: Ad[] = | |
| promotedPostsData?.organizations[0]?.advertisements?.edges.map( | |
| (edge) => edge.node, | |
| ) || []; | |
| setAdContent(ads); |
Tools
Biome
[error] 178-178: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2046 +/- ##
===========================================
- Coverage 98.43% 98.41% -0.03%
===========================================
Files 221 221
Lines 5945 5932 -13
Branches 1734 1725 -9
===========================================
- Hits 5852 5838 -14
- Misses 87 88 +1
Partials 6 6 ☔ View full report in Codecov by Sentry. |
|
@disha1202 Please resolve the coderabbit comments and the failed Test Application check. |
|
This pull request did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
|
@disha1202 please fix the failing tests |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (3)
- package.json (1 hunks)
- src/screens/UserPortal/Posts/Posts.test.tsx (2 hunks)
- src/screens/UserPortal/Posts/Posts.tsx (8 hunks)
Files skipped from review due to trivial changes (1)
- package.json
Additional context used
Biome
src/screens/UserPortal/Posts/Posts.tsx
[error] 174-174: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 303-303: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Additional comments not posted (6)
src/screens/UserPortal/Posts/Posts.tsx (5)
25-26: LGTM!The imports for
Carouseland its styles are correctly added.
28-45: LGTM!The
responsiveobject for theCarouselcomponent is correctly defined.
331-336: LGTM!The
Carouselcomponent is correctly used to display pinned posts.
Line range hint
340-347: LGTM!The
adContentis correctly mapped toPromotedPostcomponents.
174-178: Use optional chaining for safety.The use of optional chaining here can prevent potential runtime errors when
promotedPostsData.organizationsmight be undefined.However, ensure that all usages of
promotedPostsDataare updated similarly.Verification successful
All usages of
promotedPostsDataare correctly updated.The search confirms that
promotedPostsDatais used appropriately with optional chaining where necessary, and no other instances require modification.
src/screens/UserPortal/Posts/Posts.tsx: Usage in condition anduseEffectdependency array.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `promotedPostsData` are updated to use optional chaining. # Test: Search for the variable usage. Expect: Only occurrences of the new usage. rg --type js --pcre2 $'promotedPostsData\\?\\.organizations'Length of output: 59
Script:
#!/bin/bash # Description: Verify all usages of `promotedPostsData` are updated to use optional chaining. # Test: Search for all variable usages. Expect: All instances of `promotedPostsData` using optional chaining where necessary. rg --type js --pcre2 '\bpromotedPostsData\b'Length of output: 46
Script:
#!/bin/bash # Description: Find the declaration of `promotedPostsData` and inspect the surrounding code. # Test: Search for the declaration of `promotedPostsData`. Expect: Declaration and surrounding context. rg --type js --pcre2 '\bpromotedPostsData\b' -A 10 -B 10Length of output: 58
Script:
#!/bin/bash # Description: Perform a broad search for `promotedPostsData` in the entire codebase. # Test: Search for all instances of `promotedPostsData`. Expect: All instances of the variable usage. rg --pcre2 '\bpromotedPostsData\b'Length of output: 382
Tools
Biome
[error] 174-174: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/screens/UserPortal/Posts/Posts.test.tsx (1)
140-216: LGTM!The mock data for
ORGANIZATION_ADVERTISEMENT_LISTis correctly defined.
palisadoes
merged commit 9e7ebf7
into
PalisadoesFoundation:develop
What kind of change does this PR introduce?
Issue Number:
Fixes #1954
Did you add tests for your changes?
Snapshots/Videos:
test.mov
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
Summary by CodeRabbit
New Features
Improvements
togglePostPinmutation to accept atitleargument.Refactors
Bug Fixes