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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added play, pause, reboot and stop icons #4710

Merged
merged 1 commit into from
Mar 24, 2023

Conversation

lorumic
Copy link
Contributor

@lorumic lorumic commented Mar 21, 2023

Done

In the context of having icons for instance management (LXD-UI, Anbox Cloud dashboard...), this PR adds 4 new icons approved by design to the "Additional" icon set.

QA

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 馃巵, Breaking Change 馃挘, Bug 馃悰, Documentation 馃摑, Maintenance 馃敤.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix relesase (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

@webteam-app
Copy link

Demo starting at https://vanilla-framework-4710.demos.haus

Copy link
Contributor

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

LGTM, but am not too familiar with all the constraints for vanilla. Can someone more familiar with it have a 2nd eye on this one?

@lyubomir-popov
Copy link
Contributor

why the duplicates?
image

@lorumic
Copy link
Contributor Author

lorumic commented Mar 23, 2023

why the duplicates? image

The power-off/power-error duplicate was already there. I saw it and thought it was fine to have a duplicate for reboot/restart as well. If instead this is judged to be wrong, I can remove either of the reboot/restart icons and keep just one of them - the old one, probably, should be kept.

For the other duplicate, feel free to file an issue as I feel it's out of the scope of this PR, but I can remove also that one if someone instructs me about which one should be kept - this has potential implications, there might be apps relying on either of them and we would break them by removing one now.

@lyubomir-popov
Copy link
Contributor

We shouldn't be adding icons if they already exist. From the screenshot, seems both power and reboot already exist.
Can we have the 4 icons next to each other? At a quick glance, I couldnt find the play/pause stop icons

@lorumic
Copy link
Contributor Author

lorumic commented Mar 23, 2023

We shouldn't be adding icons if they already exist. From the screenshot, seems both power and reboot already exist. Can we have the 4 icons next to each other? At a quick glance, I couldnt find the play/pause stop icons

The "power" icon is different. And not really relevant to the icons added by this PR - both in terms of visual appearance and in terms of function, probably. The 4 icons next to each other are attached to this JIRA task which is currently blocked by this PR.

@lorumic
Copy link
Contributor Author

lorumic commented Mar 23, 2023

Removed the reboot icon.

Copy link
Contributor

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@bartaz bartaz merged commit d5a8e0e into canonical:main Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants