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

feat(list-editor): delete list root node (DSP-1356) #386

Merged
merged 5 commits into from Feb 17, 2021

Conversation

@mdelez
Copy link
Contributor

@mdelez mdelez commented Feb 16, 2021

resolves DSP-1356

@mdelez mdelez self-assigned this Feb 16, 2021
@mdelez mdelez requested review from kilchenmann and flavens Feb 17, 2021
@mdelez
Copy link
Contributor Author

@mdelez mdelez commented Feb 17, 2021

@kilchenmann @flavens This PR adds this button to delete a list root node
Screen Shot 2021-02-17 at 12 44 23

Loading

@kilchenmann
Copy link
Collaborator

@kilchenmann kilchenmann commented Feb 17, 2021

Looks good, but just a comment: In the ontology editor we don't use icons on the root level. There we have two buttons to edit and delete the whole ontology.

Screen Shot 2021-02-17 at 13 58 54

In the first version of list editor I also had a button with the label "Edit". We should keep it consistent...

It makes sense to have icons instead words when we do not have the space for a big button. But on this level we could have buttons with labels instead icons.

Loading

@mdelez
Copy link
Contributor Author

@mdelez mdelez commented Feb 17, 2021

In the first version of list editor I also had a button with the label "Edit". We should keep it consistent...

It makes sense to have icons instead words when we do not have the space for a big button. But on this level we could have buttons with labels instead icons.

I agree on keeping it consistent. I kinda like the icons as opposed to text since we have these icons in a few places now so the user should be familiar with them. But like you said, at this level we have space for buttons with text. I think using icons could also come to our advantage when dealing with multiple languages. Since the icon will always be the same, the way the button looks will always be the same. For example, you don't have to worry about how text in every provided language will look in regards to the CSS applied to it. Having said this, idk how easy it is to provide tooltips in multiple languages.

@flavens any opinion? If not, I'll change the buttons back to text.

Loading

@kilchenmann
Copy link
Collaborator

@kilchenmann kilchenmann commented Feb 17, 2021

In the first version of list editor I also had a button with the label "Edit". We should keep it consistent...
It makes sense to have icons instead words when we do not have the space for a big button. But on this level we could have buttons with labels instead icons.

I agree on keeping it consistent. I kinda like the icons as opposed to text since we have these icons in a few places now so the user should be familiar with them. But like you said, at this level we have space for buttons with text. I think using icons could also come to our advantage when dealing with multiple languages. Since the icon will always be the same, the way the button looks will always be the same. For example, you don't have to worry about how text in every provided language will look in regards to the CSS applied to it. Having said this, idk how easy it is to provide tooltips in multiple languages.

@flavens any opinion? If not, I'll change the buttons back to text.

I totally agree what you said but an icons will always need a text label or at least a tooltip. There are several articles about this topic e.g. https://ux.stackexchange.com/questions/1795/when-to-use-icons-vs-icons-with-text-vs-just-text-links

Anyway it's something we have to decide when do the big refactor/redesign task. This is something we have to mockup with Figma. For now we should have the same setup in all admin views. I can also convert the edit- / delete-buttons into icons in the ontology editor.

Loading

@flavens
Copy link
Collaborator

@flavens flavens commented Feb 17, 2021

@kilchenmann @mdelez Mike has a good point, with icons (and there are very common so easy to understand), we won't bother to translate the button label. Because I do not know when the interface translation will be planned in the roadmap, probably not soon. You have to stay consistent between the 2 UI view.

Loading

@kilchenmann
Copy link
Collaborator

@kilchenmann kilchenmann commented Feb 17, 2021

So, someone has to decide which type we should use 😅

Loading

@kilchenmann
Copy link
Collaborator

@kilchenmann kilchenmann commented Feb 17, 2021

I suggest to keep the icons here. I will update the view in the ontology editor. Okay?

Loading

@mdelez
Copy link
Contributor Author

@mdelez mdelez commented Feb 17, 2021

I suggest to keep the icons here. I will update the view in the ontology editor. Okay?

Okay :) I think this decision will actually have to come from Ivan, Lukas, or the Client Service Team.

Loading

@kilchenmann
Copy link
Collaborator

@kilchenmann kilchenmann commented Feb 17, 2021

Btw. I like the combination of the buttons in youtrack:

Screen Shot 2021-02-17 at 14 59 31

Loading

@mdelez mdelez changed the title feat: Delete List Root Node feat(list-editor): delete list root node (DSP-1356) Feb 17, 2021
@mdelez mdelez merged commit 5d5eabf into main Feb 17, 2021
6 checks passed
Loading
@mdelez mdelez deleted the wip/DSP-1356-delete-root-node branch Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants