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

Show remove Field and part button when it is not system defined field/part #17028

Merged
merged 4 commits into from
Nov 18, 2024
Merged

Show remove Field and part button when it is not system defined field/part #17028

merged 4 commits into from
Nov 18, 2024

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Nov 15, 2024 •
edited by hishamco
Loading

No description provided.

@MikeAlhayek MikeAlhayek marked this pull request as ready for review November 15, 2024 20:41
@MikeAlhayek MikeAlhayek changed the title Show remove Field and part button when it is not system defined field… Show remove Field and part button when it is not system defined field/part Nov 15, 2024
Piedone
Piedone reviewed Nov 16, 2024
View reviewed changes
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

While this is all good, shouldn't we have some indicator that something is a system part/field? Because I can have e.g. this:

{4B02CB77-4273-452C-808C-18AD87BF95D9}

A user may wonder, why is Dashboard not removable? Why is Html Dashboard Widget neither editable nor removable?

@hishamco
Copy link
Member

LGTM, just little notes

  1. Agree with @Piedone that we need an indicator somehow, probably a badge
  2. We could disable the button instead of hide it, just to make the buttons aligned

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Nov 18, 2024 •
edited
Loading

image

image

@hishamco
Copy link
Member

I still think the badge will be obvious to the users, also the tooltip is too long, if this could be modified I will approve now, or wait for others to approve

@MikeAlhayek
Copy link
Member Author

I used tooltip to align with the same implementation as the features UI.

Piedone
Piedone approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

Much easier to understand, thank you!

@hishamco hishamco merged commit 6e767bf into main Nov 18, 2024
17 checks passed
@hishamco hishamco deleted the ma/fis-system-defined branch November 18, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hishamco hishamco hishamco approved these changes

@Piedone Piedone Piedone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MikeAlhayek @hishamco @Piedone