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

icon and color definitions for dicom-viewer .dcm filetype #10172

Merged
merged 7 commits into from
Jan 24, 2024

Conversation

koebel
Copy link
Contributor

@koebel koebel commented Dec 14, 2023

Description

web-app-dicom-viewer is an extension to preview DICOM files (medical images) and their corresponding metadata in the browser. In order to display these .dcm files with a suitable icon in the file list, I've added a new icon. Also added two icons for the flip image function in the dicom-viewer (flip-vertical, flip-horizontal). In addition, color definitions have been added for the .dcm filetype icon.

Related Issue

Motivation and Context

I suggest to add these icons and color definitions to web's design system package in order to have all icons in one place. For all other icons used in the dicom-viewer, I reused other icons that are already part of the design system package, but for these particular cases, I couldn't find any suitable icons in the current collection. The new icons match the other icon set (same style, line type, etc.)

How Has This Been Tested?

icons and color definitions have been tested manually/visually in the browser :)

Screenshots (if appropriate):

suggested icon & color for .dcm filetype in file list
dicom-viewer-file-list

suggested icon for flip function in the dicom viewer
dicom-viewer-dcm-preview-flip-icon

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Assets added
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • follow up with RemixDesign about flip icon

@koebel koebel added Category:Enhancement Add new functionality Status:Needs-Review Needs review from a maintainer Topic:UX-Design Category:Nonfunctional Will not change behavior labels Dec 14, 2023
@koebel koebel self-assigned this Dec 14, 2023
Copy link

update-docs bot commented Dec 14, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Copy link
Contributor

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

In general this is ready to merge. Just one question: where do the flip icons come from? Did you create them yourself? We need to be clear about licenses. For the FontAwesome icons we have the attribution within the .svg source and in the browser console. If you made the icons yourself, then please add something like this in the svg files:

<!-- MIT License: https://opensource.org/licenses/MIT -->
<!-- author: https://github.com/koebel -->

(of course with whatever license you want to have...)

@koebel
Copy link
Contributor Author

koebel commented Jan 2, 2024

@kulmann I took some existing icons from your icon set (split-cells-horizontal.svg and split-cells-vertical.svg) as baseline for the flip icon and modified them myself to better fit the context of the dicom viewer functionalities. For the medical data icon, I took a font awesome icon as baseline.

@koebel
Copy link
Contributor Author

koebel commented Jan 3, 2024

@kulmann the SVGs above (split-cells-horizontal.svg and split-cells-vertical.svg) which I took as a baseline for the two flip icons from the assets provided in oc web's design-system package, don't have any attribution. Do you know anything about their source? I couldn't find them in font awesome. It looks very much like it's from this source which would have the following credentials: LICENSE: Apache License, AUTHOR: Remix Design
The Apache Licence states that you are free to remix and to adapt and so does the author. Since I just made some minor modifications, I don't feel like I'm really the author of these icons... would the following attribution be ok?

<!-- Apache License, Version 2.0: https://opensource.org/license/apache-2-0/ -->
<!-- author: Remix Icon https://github.com/Remix-Design/RemixIcon/, modified by https://github.com/koebel -->

@koebel koebel requested a review from kulmann January 3, 2024 08:58
@kulmann
Copy link
Contributor

kulmann commented Jan 3, 2024

@koebel can't you just use the two icons that we already have? split-cells-horizontal and split-cells-vertical? They look ok to me for your use case.

All our icons which are not prefixed with resource- in the file name come from https://remixicon.com - we just imported the full icon set. We still want to put an attribution in the UI (e.g. via an About page, which doesn't exist, yet). Remixicon state on their github page that attribution is not required, but I'm not happy that we don't have an attribution for those, yet. Still, we don't need to put the attribution into the .svg files.

Again, I'd prefer if you would only add the fontawesome icon for the medical files, that makes sense. But for the remixicons I'd be more happy if we wouldn't add custom icons.

Another option would be to try to get your icons upstream into remixicons, with a PR to https://github.com/Remix-Design/RemixIcon and use the cell icons for now, until remixicons (hopefully) accepts your contribution.

@koebel
Copy link
Contributor Author

koebel commented Jan 3, 2024

Thanks for the feedback. I don't think that the original split cell icon would be suitable, however there was already an issue about this in Remix Icon and I've just created a PR with the proposed solution for these icons to be added to Remix Icons.

@koebel
Copy link
Contributor Author

koebel commented Jan 17, 2024

I've removed the flip icon here while we're waiting for RemixDesign to approve the upstream PR.
@kulmann is it possible to merge this so that the icon for medical images becomes available through web?

@phil-davis
Copy link
Contributor

@koebel I restarted CI. The previous run last week had some error when building stuff in one of the pipelines, probably unrelated to this PR.
Maybe it will pass now.

@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/web/42123/7/4
It failed again in a similar way. Maybe rebase and force-push the branch.

@CLAassistant
Copy link

CLAassistant commented Jan 24, 2024

CLA assistant check
All committers have signed the CLA.

@phil-davis phil-davis force-pushed the design-elements-for-dicom-viewer branch from 7cf8343 to 6396ccb Compare January 24, 2024 05:31
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@phil-davis phil-davis merged commit b7dd5f4 into master Jan 24, 2024
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the design-elements-for-dicom-viewer branch January 24, 2024 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Enhancement Add new functionality Category:Nonfunctional Will not change behavior Status:Needs-Review Needs review from a maintainer Topic:UX-Design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants