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

deprecate: deprecate web-adaptor in favor of web-adapter #10598

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

arowles
Copy link
Contributor

@arowles arowles commented Oct 23, 2024

Related Issue: #

Summary

added
web-adaptor

@arowles arowles requested a review from aphilobos October 23, 2024 22:07
@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Oct 23, 2024
@calcite-admin calcite-admin added the skip visual snapshots Pull requests that do not need visual regression testing. label Oct 23, 2024
Copy link
Member

@benelan benelan left a comment

Choose a reason for hiding this comment

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

Thanks for adding back the deprecated icons to prevent a breaking change. Can you please change the PR title to deprecate: deprecate web-adaptor in favor of web-adapter. That will make sure it ends up on the changelog so people know to migrate to the fixed icon names.

@jcfranco
Copy link
Member

Does this also need an update to keywords.json?

@benelan
Copy link
Member

benelan commented Oct 23, 2024

Does this also need an update to keywords.json?

I think we want to remove deprecated icons from keywords.json to prevent more people from finding them in the doc and using them. The alternative is adding a deprecated field and displaying that in the doc. cc @macandcheese

@jcfranco
Copy link
Member

From a dev perspective, it’d be helpful to surface deprecation notices on the docs site. It could also help identify all icons slated for removal later. On that note, should we create an issue to track the removal of this icon for a future breaking change release?

@jcfranco
Copy link
Member

@arowles Thanks for restoring this so quickly, btw! 🎉

@benelan
Copy link
Member

benelan commented Oct 23, 2024

Yeah, maybe one issue for all of the future icon removals? We have some from #10309 too.
cc @geospatialem @DitwanP

@macandcheese can you add some logic to the icon finder for showing a deprecation chip/message?

Should it just be a boolean deprecated field, or should we add more metadata like the version of deprecation or future removal?

I think we should wait until the doc site handles deprecations before adding them back to keywords.json. Otherwise they will appear as normal icons and people may use them instead of the fixed ones.

@macandcheese
Copy link
Contributor

We could have a “deprecated in favor of” metadata field in addition to just deprecation as Boolean. We don’t need to show this to a user but we can use it to surface the new icon without showing the old name?

I also think from a use case perspective this would solve the “why isn’t my icon working”, as I’d expect only searches for exact icon term to return the deprecated ones.

We can work on the UI / UX of how we display this, I think the above metadata would be sufficient.

@arowles arowles changed the title feat: re-add web-adaptor deprecate: deprecate web-adaptor in favor of web-adapter Oct 24, 2024
Copy link
Member

@benelan benelan left a comment

Choose a reason for hiding this comment

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

Thanks!

@arowles arowles merged commit 7cdb7aa into dev Oct 24, 2024
16 checks passed
@arowles arowles deleted the arowles/add-pin-icons branch October 24, 2024 22:46
@github-actions github-actions bot added this to the 2024-10-29 - Oct Milestone milestone Oct 24, 2024
benelan added a commit that referenced this pull request Oct 25, 2024
…orepo

* origin/dev:
  build(deps): update typescript-eslint monorepo to v7.18.0 (#10593)
  feat(tile): add design tokens (#10476)
  deprecate: deprecate web-adaptor in favor of web-adapter (#10598)
  docs: update component READMEs (#10600)
  build(deps): update dependency @rollup/plugin-replace to v6 (#10604)
  chore: release main (#10597)
  ci(chromatic): build storybook even when skipping snapshots (#10589)
  build(deps): update dependency axe-core to v4.10.1 (#10566)
  ci: add calcite-ui-icons label to relevant package PRs (#10590)
  chore: release next
  feat: add buffer point, buffer polygon, buffer polyline, contour, offset (#10594)
  feat(input-date-picker, date-picker): improve date picking experience (#8402)
  chore: release next
  fix: updated web-adapter name (#10581)
  build(deps): update nrwl monorepo to v19.8.6 (#10575)
  build(deps): update dependency tsx to v4.19.1 (#10574)
  build(deps): update dependency @cspell/eslint-plugin to v8.15.4 (#10565)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues tied to a new feature or request. skip visual snapshots Pull requests that do not need visual regression testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants