fix: show entity logo actions on mobile#2161
Conversation
✅ Deploy Preview for activist-org ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Thank you for the pull request! ❤️The activist team will do our best to address your contribution as soon as we can. The following are some important points:
Note activist uses Conventional Comments in reviews to make sure that communication is as clear as possible. |
Maintainer ChecklistThe following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
First PR Commit Check
- The commit messages for the remote branch of a new contributor should be checked to make sure their email is set up correctly so that they receive credit for their contribution
- The contributor's name and icon in remote commits should be the same as what appears in the PR
- If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for
git config user.emailin their local activist repo (can be set withgit config --global user.email "GITHUB_EMAIL")
|
@aasimsyed there is some tests that are breaking? |
|
I can fix the i18n-check issues. |
|
Fixed the i18n check failure in |
|
@GHX5T-SOL I'll take a look over it tomorrow properly to see if it works but from the code I can comment some stuff |
There was a problem hiding this comment.
praise: @GHX5T-SOL thank you for your contribution. Actually this is something I thought of doing a while ago.
note: There is room for improvement since the issues are regarding making the component more maintainable with time. Remember to atomize and make the component as generic and independent as possible.
| > | ||
| <div class="flex items-center gap-3"> | ||
| <div class="relative h-16 w-16 shrink-0"> | ||
| <ImageOrganization |
There was a problem hiding this comment.
The component is over‑coupled because it hard‑codes v-if branches for each entityType (Organization, Event, Group) instead of accepting a simple, generic image source and fallback. This breaks reusability and separation of concerns.
There was a problem hiding this comment.
Fixed in 04c4a2a4. EntityLogoMobile no longer switches on entityType; the layouts now pass the image URL, fallback icon, and optional accent class, so the component stays reusable.
| ); | ||
|
|
||
| function handleEdit(): void { | ||
| if (!props.entity?.id) { |
There was a problem hiding this comment.
You can just send an emit so that it sends that edit button has been clicked
There was a problem hiding this comment.
Fixed in 04c4a2a4. The edit button now only emits edit; the event and organization layouts open ModalUploadImageIcon, and the group layout keeps the existing coming-soon toast.
|
@GHX5T-SOL for next time please don't overwrite the PR template this is important for us. |
|
Understood, sorry about that. I’ll preserve the project PR template on future contributions. |
|
Addressed the two review comments in Changes:
Validated locally:
|
nicki182
left a comment
nicki182
left a comment
There was a problem hiding this comment.
priase: thank you for making all the necessary changes it almost looks perfect, just make this small change and I will bring it in.
@GHX5T-SOL
| const props = defineProps<{ | ||
| entity: Entity | null; | ||
| accentClass?: string; | ||
| fallbackIcon: string; |
There was a problem hiding this comment.
I would change the name to icon as a prop since that's what you are passing its not a fallback is the icon if you have it
|
Updated this in What changed:
Validation run locally:
|
@andrewtavis how do you feel about the design with mobile looking like this design wise. If it its ok for you we can bring the PR in most likely if its not, we can rethink about it.
|
GHX5T-SOL
force-pushed
the
fix/2158-mobile-entity-logo
branch
from
827b809 to
d70e852
Compare
|
Rebased this on current Validation run locally after the rebase:
The same focused test without the increased hook timeout hit Nuxt setup timeout locally, so I reran it with a 30s hook timeout and it passed. |
3 participants
Summary
Fixes #2158.
Validation
corepack yarn test test/components/entity/EntityLogoMobile.spec.tscorepack yarn lintcorepack yarn typecheckcorepack yarn prettier app/components/entity/EntityLogoMobile.vue app/layouts/organization/OrganizationPage.vue app/layouts/event/EventPage.vue app/layouts/group/GroupPage.vue test/components/entity/EntityLogoMobile.spec.ts --check --config ../.prettierrc --ignore-path ../.prettierignoregit diff --cached --checkgit diff --cached | gitleaks stdin --no-banner --redact --timeout 30I also ran
corepack yarn test. It executed the suite, reporting 160 passed and 2 skipped test files with 1858 passed tests, but the command exited 1 because Vitest reported three unhandledReferenceError: window is not definederrors fromtest/components/form/FormTextEntity.spec.tsafter the tests ran. Runningcorepack yarn test test/components/form/FormTextEntity.spec.tsseparately passed 59 tests.