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

(refactor) O3-2709: Update placeholder camera icon used in registration form #1601

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

psworld
Copy link
Contributor

@psworld psworld commented Jan 19, 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

In #1580 the icon that is used for image placeholder icon did not conform to the O3- style guidelines.
I changed the placeholder icon to use Camera Icon available at https://carbondesignsystem.com/guidelines/icons/library/ under Technology section and titled Camera

Screenshots

Before

Screenshot 2024-01-19 at 09 00 55

After

camera icon

Related Issue

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

@denniskigen denniskigen changed the title (refactor): O3-2709 used Carbon Library camera icon for placeholder in patient registration (refactor) O3-2709: Update placeholder camera icon used in registration form Jan 23, 2024
Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

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

Thanks, @psworld!

@denniskigen denniskigen merged commit 8123a3a into openmrs:main Jan 23, 2024
6 checks passed
@brandones
Copy link
Contributor

Why didn't you just use the React component like we do with most of our icons? It's something like

import { Camera } from "@carbon/icons-react"

<Camera />

(but please refer to an actual usage in the codebase for the particulars).

psworld added a commit to psworld/openmrs-esm-patient-chart that referenced this pull request Jan 24, 2024
…on form (openmrs#1601)

* (refactor): O3-2709 used Carbon Library camera icon for placeholder in patient registration

* Fix indentation

* Fix Camera icon import
---------

Co-authored-by: Ps <[email protected]>
Co-authored-by: Dennis Kigen <[email protected]>
@psworld
Copy link
Contributor Author

psworld commented Jan 24, 2024

Why didn't you just use the React component like we do with most of our icons? It's something like

import { Camera } from "@carbon/icons-react"

<Camera />

(but please refer to an actual usage in the codebase for the particulars).

placeholder.svg was being used for the icon, I though that's how it should be. But now I am realizing that the custom placeholder.svg was probably being used for the Grey Rectangular Box. I totally missed it !

I have made the changes and used <Camera /> component.

Could you please guide on how should I push changes ? as this branch is already merged

  1. Should I force push changes on this same branch ? amend the last commit ?
  2. Create a new branch and push changes there ?

@denniskigen
Copy link
Member

Please create a new PR , @psworld. Thanks for catching this, @brandones.

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

Successfully merging this pull request may close these issues.

3 participants