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

Some icons improvements #4286

Merged
merged 14 commits into from
Feb 7, 2022
Merged

Some icons improvements #4286

merged 14 commits into from
Feb 7, 2022

Conversation

bsekachev
Copy link
Member

@bsekachev bsekachev commented Feb 3, 2022

Motivation and context

To clean up icons, improve UX

  1. Removed assets which are not used in CVAT, but still in the repository:
  • exit-fullscreen-icon.svg
  • object-hide-icon.svg
  • object-inside-icon.svg
  • object-occlude-icon.svg
  • plus-icon.svg
  • settings-icon.svg
  • show-sidebar-icon.svg
  • side-icon-object-lock.svg
  1. CheckOutlined replaced by CheckCircleOutlined (to be consistent with closed using of CloseCircleOutlined)
  2. CloudSyncOutlined replaced by PictureOutlined (because its meaning accoring to the code is "Preview image" and used when could not fetch preview)
  3. CloudTwoTone replaced by CloudOutlined (to be consistent with other pages projects/tasks/models/jobs. When the lists are empty, icon is outlined there, not colorful)
  4. DownloadOutlined replaced by UploadOutlined (when listing import formats for datasets)
  5. QuestionCircleFilled replaced by QuestionCircleOutlined in 2 places (because in the most of cases we use Outlined style, do not need to have two different styles)
  6. CloseOutlined replaced by DeleteOutlined (in context when meaning is removing something from a list, because opposite action has PlusCircleOutlined icon Discussed below )
  7. WarningOutlined replaced by ExclamationCircleOutlined (in modals that assumed to get confirmation from a user, because it is default antd icon for confirmation modals)

How has this been tested?

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2022 Intel Corporation
#
# SPDX-License-Identifier: MIT

@bsekachev bsekachev requested a review from azhavoro as a code owner February 4, 2022 06:05
@Marishka17
Copy link
Contributor

@bsekachev , I guess CloudTwoTone looks more beautiful and harmonizes with the button Attach a new storage
Screenshot from 2022-02-04 09-02-26

Screenshot from 2022-02-04 09-04-05

@bsekachev
Copy link
Member Author

bsekachev commented Feb 4, 2022

@Marishka17

We should be consistent with other pages:
image
image
image

There are not any color icons on other pages. What is special reason to have color here?

@Marishka17
Copy link
Contributor

@bsekachev, I only mean that it looks better. It's up to you.

@@ -42,7 +42,7 @@ export default function ConstructorViewerItem(props: ConstructorViewerItemProps)
onClick={(): void => onDelete(label)}
onKeyPress={(): boolean => false}
>
<CloseOutlined />
<MinusCircleOutlined />
Copy link
Contributor

@ActiveChooN ActiveChooN Feb 4, 2022

Choose a reason for hiding this comment

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

It looks quite strange, guess the close icon looked more convenient and less distracting.

Copy link
Member Author

@bsekachev bsekachev Feb 4, 2022

Choose a reason for hiding this comment

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

image

Add label: circled plus icon
Remove label: close icon

They are opposite actions, but icons not. One circled, another not. The same for attributes.
Following from the name of CloseOutlined it is even not supposed that the icon is used to remove something

Copy link
Member Author

Choose a reason for hiding this comment

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

But I undestand you feeling about distracting, changed icon attract your attention for now because you are not used to it yet

Copy link
Member Author

Choose a reason for hiding this comment

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

Put it here to compare
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@bsekachev , which software or products use circle with minus to remove something?

Copy link
Member Author

Choose a reason for hiding this comment

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

@nmanovic

Figma:
image

Different options are possible: minus, trash icon, close icon. But in this specific case, I believe the best two are minus (like a pair for plus icon) and trash icon (looks well with edit icon and also very intuitive))

Copy link
Member Author

Choose a reason for hiding this comment

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

For comparison:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO: Cross and trash bin look better. Minus in a circle looks like the stop sign. On Figma icons without circle and they are in one column. It makes them clear semantically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's use trash instead, I am ok with it.

@@ -329,7 +329,7 @@ export default class LabelForm extends React.Component<Props> {
this.removeAttribute(key);
}}
>
<CloseCircleOutlined />
<MinusCircleOutlined />
Copy link
Contributor

Choose a reason for hiding this comment

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

Same there.

@bsekachev bsekachev merged commit 237f98d into develop Feb 7, 2022
@bsekachev bsekachev deleted the bs/icons branch February 7, 2022 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants