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

(fix) O3-2709: wrong image placeholder used for add image in patient registration #1580

Merged
merged 3 commits into from
Jan 16, 2024

Conversation

psworld
Copy link
Contributor

@psworld psworld commented Jan 11, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

A broken image icon was used as a placeholder for the image in Add Image in patient registration. A svg palceholder.svg was being used as placeholder. I did not found any other usage or places wher this placeholder.svg is referenced, so I felt free to change it. This placeholder.svg was only being used in the capture-photo.component.tsx

Screenshots

Before

image

After

image

Image placeholder is clickable and opens the cam same as the Add image + button

Related Issue

https://openmrs.atlassian.net/browse/O3-2709

@@ -31,7 +31,7 @@ const CapturePhoto: React.FC<CapturePhotoProps> = ({ initialState, onCapturePhot
return (
<div style={{ display: 'flex', alignItems: 'center' }}>
<div style={{ maxWidth: '64px' }}>
<img src={dataUri || initialState || placeholder} alt="Preview" style={{ width: '100%' }} />
<img src={dataUri || initialState || placeholder} onClick={showCam} alt="Preview" style={{ width: '100%' }} />
Copy link
Contributor

Choose a reason for hiding this comment

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

If you make something clickable, it must either follow the link (a) contract or the button contract. See https://benmyers.dev/blog/clickable-divs/ . Or just use a <button> element with a CSS reset (this is usually the way to go).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brandones Really thankyou for the review. I didn't know about the standard practice for clickable items, thankyou for your valuable time and guidance.
I have made a new commit which incorporates the changes you suggested, kindly take a look. If more changes are required please let me know.

@brandones
Copy link
Contributor

brandones commented Jan 12, 2024

@psworld The checkboxes in the PR description are to inform reviewers what has been done and what has not. They are not strictly requirements (other than the first one). It is not helpful to check boxes that have nothing to do with your work, or which are untrue.

image

The second checkbox, about designs, is clearly false, as what you have implemented is different from the designs. The designs can be found on Zeplin. In general, do not check this box if you have not looked at the designs when doing a piece of work. So for work that involves no UI changes or for which the designs are not relevant, leave the box unchecked.

The third checkbox asks about tests. This is obviously false. It wouldn't make sense to test this change. So do not check the box.

@psworld
Copy link
Contributor Author

psworld commented Jan 12, 2024

@psworld The checkboxes in the PR description are to inform reviewers what has been done and what has not. They are not strictly requirements (other than the first one). It is not helpful to check boxes that have nothing to do with your work, or which are untrue.

image

The second checkbox, about designs, is clearly false, as what you have implemented is different from the designs. The designs can be found on Zeplin. In general, do not check this box if you have not looked at the designs when doing a piece of work. So for work that involves no UI changes or for which the designs are not relevant, leave the box unchecked.

The third checkbox asks about tests. This is obviously false. It wouldn't make sense to test this change. So do not check the box.

@brandones I will definitely keep these things in mind. Thankyou so much 🙏

@brandones brandones self-requested a review January 13, 2024 17:26
<button
type="button"
onClick={showCam}
style={{ border: 'none', maxWidth: '64px', padding: 0, margin: 0, background: 'none', cursor: 'pointer' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in a style (scss) file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brandones,
I have added a new file named capture-photo.scss and added a class named .buttonCssReset

There was no existing css file for this component nor it was importing styles from any other css file. So I created a new one

Copy link
Contributor

@brandones brandones left a comment

Choose a reason for hiding this comment

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

Style file

Copy link
Contributor

@brandones brandones left a comment

Choose a reason for hiding this comment

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

Nice work @psworld , LGTM. Welcome to the community!

@brandones brandones merged commit 8493927 into openmrs:main Jan 16, 2024
6 checks passed
@psworld
Copy link
Contributor Author

psworld commented Jan 17, 2024

Nice work @psworld , LGTM. Welcome to the community!

Thanks 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants