Skip to content

GH-22 - Changed delete character to icon in config settings component #23

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

Merged
merged 3 commits into from
Mar 19, 2020

Conversation

marianunez
Copy link
Contributor

@marianunez marianunez commented Feb 28, 2020

Summary

Changed delete character to icon in config settings component given that it was not properly rendering in Windows OS. See linked ticket.

Screenshots

Screen Shot 2020-02-29 at 12 00 12 PM

Ticket Link

Fixes #22

@marianunez marianunez added 1: UX Review Requires review by a UX Designer 2: Dev Review Requires review by a core committer labels Feb 28, 2020
@marianunez
Copy link
Contributor Author

@asaadmahmood I updated the delete character to the trash icon as suggested which looks good but needs a bit of styling, seems a bit big:

Screen Shot 2020-02-28 at 6 31 44 PM

Any suggestions to polish it?

@marianunez marianunez added the 3: QA Review Requires review by a QA tester label Feb 28, 2020
@marianunez marianunez added this to the 1.20 milestone Feb 28, 2020
@asaadmahmood
Copy link
Contributor

@marianunez Not sure why its so big, we can just reduce the font size to 14 pixels or something.

@marianunez
Copy link
Contributor Author

Font size did the trick:

Screen Shot 2020-02-29 at 12 00 12 PM

@hahmadia
Copy link
Contributor

hahmadia commented Mar 2, 2020

Hey @marianunez ! A user made a good point about the Group's which initially I had missed (#24).

Not sure if it can happen in this PR or a separate one but it would be a good idea to hide the Groups field if the user doesn't have E20 license. In addition to this, it would be a good idea to update the README to mention how applying attribute to via Group ID is a feature only available to E20 customers and doesn't apply to others (to avoid the confusion mentioned in the ticket). Thanks!

@marianunez
Copy link
Contributor Author

@hahmadia Sure. If you can point me how it should be checked for E20 (not familiar with licensing checks) I can go ahead and add it.

@hahmadia
Copy link
Contributor

hahmadia commented Mar 2, 2020

@hahmadia Sure. If you can point me how it should be checked for E20 (not familiar with licensing checks) I can go ahead and add it.

No worries. I think you can finish this PR and when I am back from PTO, I will create a new PR to address this issue. I am not currently available and wouldn't want to hold you back on merging this PR. Thanks!

@DHaussermann
Copy link

Good catch @hahmadia!

I have created a separate issue here #27 to address hiding the Group ID field when it is not applicable.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Icon replaced with trash
  • Tested on Windows and Mac to ensure the icon displays correctly
    LGTM! Thanks @marianunez

Will test again if any changes are requested from code review.

@DHaussermann DHaussermann added QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels Mar 3, 2020
@DHaussermann
Copy link

@asaadmahmood I tested this after the icons were reduced in size. Did you want to review this again?

@marianunez marianunez requested a review from jfrerich March 6, 2020 14:39
@aaronrothschild
Copy link
Contributor

aaronrothschild commented Mar 13, 2020

Hey @jfrerich Can we get this updated to pass Github ready for release? cc @DHaussermann @asaadmahmood

@jfrerich
Copy link
Contributor

@aaronrothschild, I just approved.

@jfrerich jfrerich removed the 2: Dev Review Requires review by a core committer label Mar 13, 2020
Copy link
Contributor

@jfrerich jfrerich left a comment

Choose a reason for hiding this comment

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

LGTM!

@levb levb merged commit a36ac72 into master Mar 19, 2020
@levb levb deleted the delete-icon-fix branch March 19, 2020 15:59
@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 1: UX Review Requires review by a UX Designer labels Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete icon in admin configuration UI looks odd
8 participants