Remove `Pds:Icon` entirely from Structure

didoo commented 2 years ago

Description

This is a follow-up of https://github.com/hashicorp/structure/pull/126 in which we completely remove the Pds::Icon component from Structure, and with it also some of its dependencies not needed anymore (@hashicorp/structure-icons and ember-svg-jar).

Changes

In this PR I have:

Removed the Pds::Icon component
- updated a few components that were consuming (via @use) the layout/appearance styles of Pds::Icon (GlobalFooter and RadioField::Facade)
added Storybook documentation files for deprecated Pds::Icon component
- preview: https://structure-git-remove-pds-icon-entirely-hashicorp.vercel.app/?path=/story/deprecated-icon-deprecated--page
removed ember-svg-jar as dependency
removed @hashicorp/structure-icons as dependency

Testing

Not possible to test until all the instances of Pds::Icon in Cloud UI have been migrated to FlightIcon

Relevant links

Epic in Jira: https://hashicorp.atlassian.net/browse/HDS-670?filter=10826

🚧 TODOs

Once this PR is merged and released as new Structure version, in Cloud UI it will be necessary to:

make sure that there's no usage of Pds::Icon in the codebase
understand if we want to remove the usage of svg-jar in Cloud UI (it's used only in three places, and they could be replaced with Flight icons)
only once these have been "fixed" it will be possible to bump the version of Structure to this new one

vercel[bot] commented 2 years ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
structure	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 22, 2023 at 11:24AM (UTC)

jesdavpet commented 1 year ago

"... understand if we want to remove the usage of svg-jar in Cloud UI ..."

Yes @didoo , we will ultimately remove ember-svg-jar from Cloud UI. However, this work has not been prioritized yet, you can follow/comment/link the Jira ticket below.

SEE: https://hashicorp.atlassian.net/browse/HCE-706

jesdavpet commented 1 year ago

"... (it's used only in three places, and they could be replaced with Flight icons) ..."

No @didoo , I don't think we can use Flight icons to replace these actually, because Flight doesn't have a way to represent the branded logo + product names as in the design.

SEE: template invocations of the {{svg-jar ...}} Ember helper for non-icon SVGs

https://github.com/hashicorp/cloud-ui/blob/6e5d69fd2c8809a918df2c985afa2cc37d92efce/hcp/app/components/hcp-marketing/index.hbs#L26-L51

SEE: SVG resources referenced through the {{svg-jar ...}} Ember helper

didoo commented 1 year ago

@didoo + @hashicorp/design-systems thanks for making this change to Structure!

@jesdavpet thank you for commenting on this, and sorry if it took me so long to reply to this (it's been in my todo list for over one month!)

Below my responses/comments.

Is it fair to assume that these breaking changes will be released in a semantically versioned "major" bump of Structure? Yes.

If these changes will be released as part of a major release, then I think we don't need to block releasing this version of the library by waiting for Cloud UI to complete all of the preparation to adopt. The problem I see is that, if we release this, then we can't release other potentially minor (or even major) version for changes that we know will not impact on Cloud UI (eg. removing components that are not used anymore in Cloud UI). Instead, unfortunately, Pds::Icon is still widely used in Cloud UI. Not directly (there's only one instance left) but via the <Icon> component (packages/cloud-ui-core/addon/components/icon/index.hbs) which has roughly 100 instances still to be removed. I know @cbfx had a plan to migrate/remove these instances, but I don't know what is the status if that epic (probably more urgent things, I imagine).

So, the plan on our side is this one:

finish the work we're doing around the new HDS website (top very-high priority)
in February, post launch, I'll take stock of what is the status if removal of PDS components in Cloud UI (included the App-related components, not not used anymore with the new navigation, see https://github.com/hashicorp/cloud-ui/pull/4042) and make a plan to do a release that will remove all the components that I know for sure are not used anymore in Cloud UI from Structure, and release a new version (major/minor tbd). Once done this, we can sit down (me, you, @cbfx and any other stakeholder) and plan a way to do the last mile of this effort to remove old Structure icons from Cloud UI.

Seems a plan to me, what do you think?

/cc @Dhaulagiri

jesdavpet commented 1 year ago

Thanks for elaborating on the version release considerations around PDS for this change.

"The problem I see is that, if we release this, then we can't release other potentially minor (or even major) version for changes ..."

Yes, I agree that this change should be part of a major version bump to conform to semantic versioning, since it is a breaking change. 👍 Seems OK if the next major version comes a little later, since Cloud UI won't be ready to for this particular change yet, anyhow.

"Seems a plan to me, what do you think?"

@didoo + @Dhaulagiri IMO we will probably get a better result if we avoid planning aspects of the HDS migration project in GitHub comments, please feel free to raise this topic (again, I know) in the Experiences Guild meeting, or with our (shared) PM Misha Dhar.

Dhaulagiri commented 1 year ago

@jesdavpet great point. I'll put this on my agenda to review with him.

hashicorp / structure