-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Refactor: remaining components with profileavatar - 1 #6226
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
base: develop
Are you sure you want to change the base?
Changes from 4 commits
a69977e
2783918
4f6431b
1722632
ec818ba
2900e3a
f9c18e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,7 +26,7 @@ | |||||||||||||||||||||||||||||
| * Success or error messages are displayed using toast notifications based on the result of the mutation. | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| import type { ChangeEvent } from 'react'; | ||||||||||||||||||||||||||||||
| import { Button, Form, Modal } from 'react-bootstrap'; | ||||||||||||||||||||||||||||||
| import { Button, Form } from 'react-bootstrap'; | ||||||||||||||||||||||||||||||
| import type { | ||||||||||||||||||||||||||||||
| InterfaceCreateVolunteerGroup, | ||||||||||||||||||||||||||||||
| InterfaceVolunteerGroupInfo, | ||||||||||||||||||||||||||||||
|
|
@@ -56,8 +56,9 @@ import { | |||||||||||||||||||||||||||||
| import { PiUserListBold } from 'react-icons/pi'; | ||||||||||||||||||||||||||||||
| import { TbListDetails } from 'react-icons/tb'; | ||||||||||||||||||||||||||||||
| import { USER_VOLUNTEER_MEMBERSHIP } from 'GraphQl/Queries/EventVolunteerQueries'; | ||||||||||||||||||||||||||||||
| import Avatar from 'components/Avatar/Avatar'; | ||||||||||||||||||||||||||||||
| import { FaXmark } from 'react-icons/fa6'; | ||||||||||||||||||||||||||||||
| import { ProfileAvatarDisplay } from 'shared-components/ProfileAvatarDisplay/ProfileAvatarDisplay'; | ||||||||||||||||||||||||||||||
| import BaseModal from 'shared-components/BaseModal/BaseModal'; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| export interface InterfaceGroupModal { | ||||||||||||||||||||||||||||||
| isOpen: boolean; | ||||||||||||||||||||||||||||||
|
|
@@ -187,19 +188,14 @@ const GroupModal: React.FC<InterfaceGroupModal> = ({ | |||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||
| <Modal className={styles.groupModal} onHide={hide} show={isOpen}> | ||||||||||||||||||||||||||||||
| <Modal.Header> | ||||||||||||||||||||||||||||||
| <p className={styles.titlemodal}>{t('manageGroup')}</p> | ||||||||||||||||||||||||||||||
| <Button | ||||||||||||||||||||||||||||||
| variant="danger" | ||||||||||||||||||||||||||||||
| onClick={hide} | ||||||||||||||||||||||||||||||
| className={styles.modalCloseBtn} | ||||||||||||||||||||||||||||||
| data-testid="modalCloseBtn" | ||||||||||||||||||||||||||||||
| > | ||||||||||||||||||||||||||||||
| <i className="fa fa-times"></i> | ||||||||||||||||||||||||||||||
| </Button> | ||||||||||||||||||||||||||||||
| </Modal.Header> | ||||||||||||||||||||||||||||||
| <Modal.Body> | ||||||||||||||||||||||||||||||
| <BaseModal | ||||||||||||||||||||||||||||||
| className={styles.groupModal} | ||||||||||||||||||||||||||||||
| show={isOpen} | ||||||||||||||||||||||||||||||
| onHide={hide} | ||||||||||||||||||||||||||||||
| title={t('manageGroup')} | ||||||||||||||||||||||||||||||
| data-testid="groupModal" | ||||||||||||||||||||||||||||||
| > | ||||||||||||||||||||||||||||||
|
Comment on lines
+191
to
+197
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ Fix incorrect prop name: Line 196 uses 🔧 Proposed fix <BaseModal
className={styles.groupModal}
show={isOpen}
onHide={hide}
title={t('manageGroup')}
- data-testid="groupModal"
+ dataTestId="groupModal"
>📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
| <> | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Remove unnecessary fragment wrapper. The ♻️ Proposed refactor <BaseModal
className={styles.groupModal}
show={isOpen}
onHide={hide}
title={t('manageGroup')}
dataTestId="groupModal"
>
- <>
<div
className={`btn-group ${styles.toggleGroup} mt-0 px-3 mb-4 w-100`}
role="group"
>
{/* ...rest of content... */}
</div>
- </>
</BaseModal>Also applies to: 393-393 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
| <div | ||||||||||||||||||||||||||||||
| className={`btn-group ${styles.toggleGroup} mt-0 px-3 mb-4 w-100`} | ||||||||||||||||||||||||||||||
| role="group" | ||||||||||||||||||||||||||||||
|
|
@@ -349,24 +345,14 @@ const GroupModal: React.FC<InterfaceGroupModal> = ({ | |||||||||||||||||||||||||||||
| className="d-flex gap-1 align-items-center" | ||||||||||||||||||||||||||||||
| data-testid="userName" | ||||||||||||||||||||||||||||||
| > | ||||||||||||||||||||||||||||||
| {avatarURL ? ( | ||||||||||||||||||||||||||||||
| <img | ||||||||||||||||||||||||||||||
| src={avatarURL} | ||||||||||||||||||||||||||||||
| alt={t('volunteerAlt')} | ||||||||||||||||||||||||||||||
| data-testid={`image${id + 1}`} | ||||||||||||||||||||||||||||||
| className={styles.TableImage} | ||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||
| ) : ( | ||||||||||||||||||||||||||||||
| <div className={styles.avatarContainer}> | ||||||||||||||||||||||||||||||
| <Avatar | ||||||||||||||||||||||||||||||
| key={id + '1'} | ||||||||||||||||||||||||||||||
| containerStyle={styles.imageContainer} | ||||||||||||||||||||||||||||||
| avatarStyle={styles.TableImage} | ||||||||||||||||||||||||||||||
| name={name} | ||||||||||||||||||||||||||||||
| alt={name} | ||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||
| <ProfileAvatarDisplay | ||||||||||||||||||||||||||||||
| key={id + '1'} | ||||||||||||||||||||||||||||||
| imageUrl={avatarURL} | ||||||||||||||||||||||||||||||
| fallbackName={t('volunteerAlt')} | ||||||||||||||||||||||||||||||
| dataTestId={'image' + (id + 1)} | ||||||||||||||||||||||||||||||
| className={styles.TableImage} | ||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||
|
Comment on lines
+348
to
+354
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial 🧩 Analysis chain🏁 Script executed: # Check for translation keys in locale files
for locale in en es fr hi zh; do
echo "=== Checking $locale ==="
jq '.eventVolunteers.volunteerAlt // "NOT_FOUND"' "public/locales/$locale/translation.json" 2>/dev/null || echo "File or key not found"
doneRepository: PalisadoesFoundation/talawa-admin Length of output: 210 🏁 Script executed: # Check the GroupModal.tsx file context around lines 348-354
sed -n '340,360p' src/screens/UserPortal/Volunteer/Groups/GroupModal.tsxRepository: PalisadoesFoundation/talawa-admin Length of output: 1001 🏁 Script executed: # Verify ProfileAvatarDisplay import and usage patterns in the file
grep -n "ProfileAvatarDisplay" src/screens/UserPortal/Volunteer/Groups/GroupModal.tsx | head -20Repository: PalisadoesFoundation/talawa-admin Length of output: 233 🏁 Script executed: # Check locale files more directly for volunteerAlt key
echo "=== Checking all locale files for 'volunteerAlt' key ==="
for locale in en es fr hi zh; do
file="public/locales/$locale/translation.json"
if [ -f "$file" ]; then
echo "--- $locale ---"
grep -o '"volunteerAlt"' "$file" || echo "KEY NOT FOUND"
else
echo "File not found: $file"
fi
doneRepository: PalisadoesFoundation/talawa-admin Length of output: 338 🏁 Script executed: # Check what translation keys are available in the eventVolunteers section
echo "=== Checking eventVolunteers section in English locale ==="
jq '.eventVolunteers' public/locales/en/translation.jsonRepository: PalisadoesFoundation/talawa-admin Length of output: 2807 🏁 Script executed: # Verify the complete GroupModal context to understand i18n setup
sed -n '1,70p' src/screens/UserPortal/Volunteer/Groups/GroupModal.tsxRepository: PalisadoesFoundation/talawa-admin Length of output: 2797 Translation key verified; consider using template literals for consistency. The For code consistency, consider replacing string concatenation with template literals:
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| {name} | ||||||||||||||||||||||||||||||
| </TableCell> | ||||||||||||||||||||||||||||||
| <TableCell component="th" scope="row"> | ||||||||||||||||||||||||||||||
|
|
@@ -404,8 +390,8 @@ const GroupModal: React.FC<InterfaceGroupModal> = ({ | |||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||
| </Modal.Body> | ||||||||||||||||||||||||||||||
| </Modal> | ||||||||||||||||||||||||||||||
| </> | ||||||||||||||||||||||||||||||
| </BaseModal> | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| export default GroupModal; | ||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Consider testing translated alt text value instead of i18n key.
The test queries for the literal i18n key
'profileAvatar.altText'rather than a translated value. While this works because the mock translation function returns keys for untranslated strings, it tests an implementation detail rather than user-facing behavior.💡 Suggested improvement
Add the translation to the mock and test for the actual translated value:
const mockTImplementation = (key: string) => { const translations: Record<string, string> = { talawaAdminPortal: 'Talawa Admin Portal', adminPortal: 'Admin Portal', menu: 'Menu', plugins: 'Plugins', Dashboard: 'Dashboard', Members: 'Members', Events: 'Events', 'Action Items': 'Action Items', Posts: 'Posts', + 'profileAvatar.altText': 'Organization Avatar', }; return translations[key] || key; };Then update the test:
This makes the test more robust to i18n key refactoring and better reflects actual user experience.
🤖 Prompt for AI Agents