-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Fixing image that isn't displaying properly. #57428
Conversation
Waiting for the build and then I'll review to see if the image is working. |
Concierge reviewer checklist:
For more detailed instructions on completing this checklist, see How do I review a HelpDot PR as a Concierge Team member? |
@NikkiWines @jliexpensify One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
A preview of your ExpensifyHelp changes have been deployed to https://39744ad0.helpdot.pages.dev ⚡️ |
Deleted and readded the image code hoping to fix the issue.
Okay made a change, trying again. |
🤔 - still broken and I don't know why. |
@strepanier03 I don't think we want to inlcude the .vscode/settings.json file that's been added |
Removing some code that I'm not even sure how it got added.
@NikkiWines - I'm not even sure how that got added, I definitely didn't do it manually. Maybe it's my github.dev, it's been acting really funny the last week with connection errors. I removed it and am letting the build go again. If it doesn't work then I think I might close this and start over entirely. |
@strepanier03 I'm happy to help out here too if you want me to take it over |
I'm guessing I shoudl wait for the Concierge review? |
@NikkiWines - I'd love to learn how to fix the image issue and why it won't load the image from the repo on th site or in the build. This only happens if I try to add the image to the help doc in the same PR I add it to the repo. I normally don't do it in one PR but was asked to for testing purposes and to see if it was possible. |
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.
@strepanier03 it looks like the format of the image file isn't correct in GH. If you go to https://github.com/Expensify/App/blob/main/docs/assets/images/Edit_Photo.png you can see the image isn't rendering correctly in GH even on main
, so it's not tied to the changes in this PR (which I guess is obvious since you're making this PR to fix the initial issue).
It looks like we might need to delete the old image and re-upload a new one (we can do that as part of this PR), maybe the rename caused issues
Also looks like the .vscode/settings.json file is still in the PR, we should delete that 👍 |
Ok, it's a little weird using Github's UI, so I made a video on how to add and delete the photo on your branch - hopefully this helps! Screen.Recording.2025-02-27.at.23.34.29.mov |
Thank you @NikkiWines I will tackle this tomorrow. I really appreciate your help understanding how to correct this and for you taking the time to teach me. |
Gonna snag this one to get it finished real quick! |
I don't know how to get it to regenerate the preview for the help site, but it should be all good now! https://github.com/Expensify/App/blob/strepanier03-editphoto-fix/docs/assets/images/Edit_Photo.png <- this now shows the photo rendering correctly 🎊 |
She committed code to the PR so I'll take over review
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.
https://github.com/Expensify/App/blob/strepanier03-editphoto-fix/docs/assets/images/Edit_Photo.png <- this now shows the photo rendering correctly 🎊
Things look good there! Although from the code review pane it's saying Invalid image source.
🤔 Not really sure if that's a lie or the truth haha but either way it's not going to be more broken than it was before. So let's give it a shot!
Explanation of Change
Tryin to fix the display of the image added to this doc. From here - https://github.com/Expensify/Expensify/issues/439560
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop