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

[full-ci] Use icon from app registered for the file type #9088

Merged
merged 10 commits into from
Jun 9, 2023

Conversation

dschmidt
Copy link
Member

@dschmidt dschmidt commented May 23, 2023

Description

Use app icons as resource icons.

Motivation and Context

Apps can already specify an icon (with filltype and color), but it's not used for resource icons.
If a filetype does not exist in our default mappings, we just show a default file icon although we could (and want to) show the app icon.

How Has This Been Tested?

With my gpx viewer app. See screenshots and the definition in code here:
https://github.com/dschmidt/web-app-gpx-viewer/blob/main/src/index.ts#L12L14

Screenshots (if appropriate):

Notice the file icon which is a green map

Screenshot_20230608_150116
Screenshot_20230608_150233

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
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • handle priorities
  • merge resourceIconColorExtensionMapping.json and resourceIconExtensionMapping.json to one file implementing OcResourceIconMapping
  • tests 🙈
  • the default mapping misses colors for some file types, should we add them? I could not find any reason why they should be missing

Follow up tasks

  • add mimetypes to the default mapping
  • handle icons per file type in apps/extensions

@update-docs
Copy link

update-docs bot commented May 23, 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.

@dschmidt dschmidt mentioned this pull request May 23, 2023
30 tasks
@dschmidt dschmidt force-pushed the file-icons-from-apps branch 2 times, most recently from 88a2597 to db01090 Compare June 8, 2023 12:54
@dschmidt dschmidt changed the title Use icon from app registered for the file type [full-ci] Use icon from app registered for the file type Jun 8, 2023
@dschmidt dschmidt marked this pull request as ready for review June 8, 2023 14:01
Copy link
Member

@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.

super nice! two nitpicks for possible improvements (see comments), but already works like a charm. 🚀

},
"accdb": {
"name": "resource-type-file",
"color": "var(--oc-color-text-default)"
Copy link
Member

Choose a reason for hiding this comment

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

ultra nit pick of the day: you could remove the color from all icon mappings where the value is "var(--oc-color-text-default)" since your implementation handles the default color as fallback if no color is defined 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

I would prefer to handle that in a follow up at some point, when we add more mlmetype handling

@sonarcloud
Copy link

sonarcloud bot commented Jun 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

59.2% 59.2% Coverage
0.0% 0.0% Duplication

@dschmidt dschmidt enabled auto-merge (squash) June 9, 2023 10:03
@dschmidt dschmidt merged commit f75f501 into master Jun 9, 2023
@delete-merged-branch delete-merged-branch bot deleted the file-icons-from-apps branch June 9, 2023 10:22
@micbar micbar mentioned this pull request Jul 24, 2023
68 tasks
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.

2 participants