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.

Sign up for GitHub

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

Jump to bottom

feat: change default icon for unknown entities #21967

Merged
merged 1 commit into from
Jan 23, 2024
Merged

feat: change default icon for unknown entities #21967

merged 1 commit into from
Jan 23, 2024

Conversation

dweber019
Copy link
Contributor

Hey, I just made a Pull Request!

Raised in #21900.

Here some screenshots of the implementation.
Screen Shot 2023-12-21 at 09 03 06 AM
Screen Shot 2023-12-21 at 09 30 58 AM
Screen Shot 2023-12-21 at 09 31 08 AM
Screen Shot 2023-12-21 at 09 48 15 AM
Screen Shot 2023-12-21 at 09 57 52 AM
Screen Shot 2023-12-21 at 09 58 05 AM

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@dweber019 dweber019 requested review from a team as code owners December 21, 2023 13:01
@github-actions github-actions bot added the area:catalog Related to the Catalog Project Area label Dec 21, 2023
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Dec 21, 2023 •
edited
Loading

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-catalog-graph plugins/catalog-graph patch v0.3.3
@backstage/plugin-catalog-import plugins/catalog-import patch v0.10.5
@backstage/plugin-catalog-react plugins/catalog-react patch v1.9.3
@backstage/plugin-catalog plugins/catalog patch v1.16.1
@backstage/plugin-entity-validation plugins/entity-validation patch v0.1.14

@dweber019 dweber019 mentioned this pull request Dec 21, 2023
Merged
5 tasks
@benjdlambert
Copy link
Member

Not for this PR, but thinking that we should drop the double Icon in some of the screenshots :)

@dweber019
Copy link
Contributor Author

I will remove the double icons, they look odd 😉

@dweber019
Copy link
Contributor Author

Updated the entity inspect screens to remove double icons. Now they only use the presentation api.
Screen Shot 2023-12-21 at 14 27 40 PM
Screen Shot 2023-12-21 at 14 28 00 PM

@github-actions GitHub Actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

@github-actions github-actions bot added the stale label Dec 28, 2023
@freben freben removed the stale label Dec 28, 2023
tudi2d
tudi2d suggested changes Jan 2, 2024
View reviewed changes
Copy link
Member

@tudi2d tudi2d left a comment

Choose a reason for hiding this comment

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

Just a first feedback. I'm curious why you prefer rendering an empty icon with SVGIcon rather than conditionally rendering it or returning null, when there is no Icon set. Is it because of keeping the type consistend or avoiding breakages in case someone uses those components somewhere else? :)

plugins/catalog-graph/src/components/EntityRelationsGraph/DefaultRenderNode.tsx Outdated Show resolved Hide resolved
plugins/catalog-import/src/components/EntityListComponent/EntityListComponent.tsx Outdated Show resolved Hide resolved
plugins/catalog-react/src/components/InspectEntityDialog/components/AncestryPage.tsx Outdated Show resolved Hide resolved
@dweber019
Copy link
Contributor Author

Thanks @tudi2d

Is it because of keeping the type consistend or avoiding breakages in case someone uses those components somewhere else?

Actually both, if someone is using EntityKindIcon it should not break and behave consistent.

@dweber019 dweber019 force-pushed the feat/change-default-icon branch 2 times, most recently from 1266525 to d8ef3da Compare January 9, 2024 20:55
@Rugvip Rugvip force-pushed the master branch 2 times, most recently from ec8028e to 0ea52bd Compare January 9, 2024 23:08
@dweber019 dweber019 requested review from Xantier and a team as code owners January 9, 2024 23:08
@dweber019 dweber019 requested a review from a team as a code owner January 9, 2024 23:08
@benjdlambert
Copy link
Member

@dweber019 I think this needs a rebase, we had to make some fixes to mainline branch yesterday and some PR's might have got caught up in the change.

@dweber019
Copy link
Contributor Author

@benjdlambert did the rebase but e2e tests still failing.
Can I do something?

@benjdlambert
Copy link
Member

benjdlambert commented Jan 11, 2024 •
edited
Loading

@dweber019 yeah we've hit a bit a rough patch with some upstream dependencies breaking our e2e test with some typescript issues - we're thinking about how we can make this a little better. mui/material-ui#40427

I think that this is looking pretty good - just wanna get some eyes from @backstage/catalog-maintainers.

alde
alde approved these changes Jan 17, 2024
View reviewed changes
Copy link
Collaborator

@alde alde left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 18, 2024 •
edited
Loading

Uffizzi Cluster pr-21967 was deleted.

@dweber019 @freben
feat: change default icon for unknown entities
916da47 
Signed-off-by: David Weber <david.weber@w3tec.ch>
@freben
Copy link
Member

freben commented Jan 23, 2024

rebased this to see if we can get it merged

tudi2d
tudi2d approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@tudi2d tudi2d left a comment

Choose a reason for hiding this comment

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

let's get this in - thanks for your contribution!:)

@freben freben merged commit 5bfbfa2 into backstage:master Jan 23, 2024
27 of 28 checks passed
@github-actions GitHub Actions
Copy link
Contributor

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.23.0 release, scheduled for Tue, 20 Feb 2024.

@dweber019 dweber019 deleted the feat/change-default-icon branch January 23, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alde alde alde approved these changes

@tudi2d tudi2d tudi2d approved these changes

@freben freben Awaiting requested review from freben freben is a code owner automatically assigned from backstage/maintainers

@benjdlambert benjdlambert Awaiting requested review from benjdlambert benjdlambert was automatically assigned from backstage/scaffolder-maintainers

Assignees
No one assigned
Labels
area:catalog Related to the Catalog Project Area
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dweber019 @benjdlambert @freben @alde @tudi2d