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

Use trash icon from FontAwesome for delete command operation #7951

Merged
merged 2 commits into from
Dec 20, 2017

Conversation

vzhukovs
Copy link
Contributor

What does this PR do?

This changes proposal replaces current icon for delete command button with the new one used from Font Awesome:

image

Signed-off-by: Vladyslav Zhukovskyi vzhukovs@redhat.com

What issues does this PR fix or reference?

openshiftio/openshift.io#1502

Release Notes

N/A

Docs PR

N/A

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
@vzhukovs vzhukovs added kind/task Internal things, technical debt, and to-do tasks to be performed. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. target/che6 labels Dec 19, 2017
@vzhukovs vzhukovs added this to the 6.0.0-M4 milestone Dec 19, 2017
@vzhukovs vzhukovs self-assigned this Dec 19, 2017
Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
@vzhukovs
Copy link
Contributor Author

ci-build

@dkuleshov
Copy link

Hi,

Everything looks good, but I wonder what you think about the next thing. I'd rather added a separate method createButton(String) instead of making common method getIconElement, because though technically it is permissible, it seems to me a bit weird when you do icon instanceof String, because essentially icon and string are quite different entities. Or, by the way, we can have createButton(Element) and createButtonElement(SVGResource resource) and createButtonElement(String iHtmlTag)?

@codenvy-ci
Copy link

@vzhukovs
Copy link
Contributor Author

@dkuleshov it's not a problem to refactor this PR. The main reason why I'm comparing with String is that we have org.eclipse.che.ide.FontAwesome class where icons described as html strings. There isn't any impediments to create two separate private methods to process two types of icon.

@vzhukovs vzhukovs merged commit d4ab115 into che6 Dec 20, 2017
@vzhukovs vzhukovs deleted the openshift#1502 branch December 20, 2017 08:57
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Dec 20, 2017
@slemeur
Copy link
Contributor

slemeur commented Dec 20, 2017

@vzhukovskii : Have you handle "deleting" using the keyboard ?

@slemeur
Copy link
Contributor

slemeur commented Dec 20, 2017

This does not fix: #7715

@vzhukovs
Copy link
Contributor Author

@slemeur no, this PR just changes the icon with the new one.

@slemeur
Copy link
Contributor

slemeur commented Dec 20, 2017

so where are the other improvementS?

@ashumilova
Copy link
Contributor

The PR handled the description of the following issue only - openshiftio/openshift.io#1502
this one will take into account - #7715

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
No open projects
IDE
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

8 participants