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

Remove Ubuntu and Canonical social icons #2683

Merged
merged 9 commits into from Dec 4, 2019

Conversation

kwm14
Copy link
Contributor

@kwm14 kwm14 commented Dec 2, 2019

Done

  • Removed Ubuntu and Canonical icons from the social section, as they aren't actually social icons so aren't relevant in this section. If we were to drive users to the social content of Ubuntu or Canonical it would be via Facebook, Twitter, etc...

QA

Details

Fixes #2574

Screenshots

Screenshot 2019-12-02 at 16 15 59

@webteam-app
Copy link

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.

LGTM

@anthonydillon
Copy link
Contributor

This will be a breaking change for sites using canonical and Ubuntu icons.

@bartaz
Copy link
Contributor

bartaz commented Dec 3, 2019

@anthonydillon

This will be a breaking change for sites using canonical and Ubuntu icons.

Exactly, I wanted to ask about that.

@kwm14 @deadlight
Do we know who may be using this? It seems that these icons are there in Vanilla for a while.
How do we keep track of changes like this to add them to release notes?

Should we first deprecate it (hide from docs, but keep in code)?
And remove when major version is released?

@anthonydillon
Copy link
Contributor

You can search GitHub for example: https://github.com/search?q=org%3Acanonical-web-and-design+p-icon--canonical&type=Code

I would suggest these icons should be deprecated for now.

@@ -32,8 +32,6 @@ $social-icon-size: map-get($icon-sizes, heading-icon--small);
@include vf-p-icon-instagram;
@include vf-p-icon-linkedin;
@include vf-p-icon-youtube;
@include vf-p-icon-canonical;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to keep @include vf-p-icon-canonical; @include vf-p-icon-ubuntu; to make sure these icons are still part of Vanilla for backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes. Will add back in 👍

@bartaz
Copy link
Contributor

bartaz commented Dec 3, 2019

While adding deprecate to SCSS makes sure we get warnings when building Vanilla, we should probably also make it visible in docs that these classes are deprecated?

If someone is using vanilla directly from CSS (not build it on their own), they may not notice the deprecation from the code. It should also be reflected in the docs.

Unless we treat removing them from the docs a direct indication that they shouldn't be used?

@kwm14
Copy link
Contributor Author

kwm14 commented Dec 3, 2019

While adding deprecate to SCSS makes sure we get warnings when building Vanilla, we should probably also make it visible in docs that these classes are deprecated?

Good idea. I can add them to our component status table - https://docs.vanillaframework.io/component-status#current-status and add 'Deprecated' label alongside the heading similar to https://docs.vanillaframework.io/patterns/menu-button/

@kwm14
Copy link
Contributor Author

kwm14 commented Dec 3, 2019

@bartaz added depreacted label to social icons heading and also added to our current status table.

@bartaz
Copy link
Contributor

bartaz commented Dec 3, 2019

@kwm14 Thanks.

I think that deprecation label in social icon docs looks a bit misleading - like the whole set of social icons is deprecated:

Screenshot 2019-12-03 at 12 48 47

Instead of showing "Deprecated" label right after the title, could we just add a note after the icons example that would say something like "Deprecated: p-icon-canonical and p-icon-ubuntu are deprecated and will be removed in future version of Vanilla" - similar to component status table?

@kwm14
Copy link
Contributor Author

kwm14 commented Dec 3, 2019

@kwm14 Thanks.

I think that deprecation label in social icon docs looks a bit misleading - like the whole set of social icons is deprecated:

Valid point 😉

Instead of showing "Deprecated" label right after the title, could we just add a note after the icons example that would say something like "Deprecated: p-icon-canonical and p-icon-ubuntu are deprecated and will be removed in future version of Vanilla" - similar to component status table?

Will update 👍

docs/patterns/icons.md Outdated Show resolved Hide resolved
Co-Authored-By: Bartek Szopka <83575+bartaz@users.noreply.github.com>
@bartaz
Copy link
Contributor

bartaz commented Dec 4, 2019

@kwm14 It doesn't seem related to this PR, but p-icon-rss and p-icon-email are empty in the docs, is this expected?

Screenshot 2019-12-04 at 09 53 41

@kwm14
Copy link
Contributor Author

kwm14 commented Dec 4, 2019

@kwm14 It doesn't seem related to this PR, but p-icon-rss and p-icon-email are empty in the docs, is this expected?

Screenshot 2019-12-04 at 09 53 41

Yes, it's from the previous PR. New icons won't show it the Docs as it's not running on the latest version

@kwm14
Copy link
Contributor Author

kwm14 commented Dec 4, 2019

@kwm14 It doesn't seem related to this PR, but p-icon-rss and p-icon-email are empty in the docs, is this expected?
Screenshot 2019-12-04 at 09 53 41

Yes, it's from the previous PR. New icons won't show it the Docs as it's not running on the latest version 👎it will show once we update inline with release.

@bartaz bartaz merged commit 0a05979 into master Dec 4, 2019
@bartaz bartaz deleted the remove-ubuntu-and-canonical-icon branch December 4, 2019 09:26
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.

Remove Ubuntu and Canonical social icons
4 participants