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.

Sign up for GitHub

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

Jump to bottom

Deprecate the className prop #799

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Deprecate the className prop #799

wants to merge 4 commits into from

Conversation

remcohaszing
Copy link
Member

@remcohaszing remcohaszing commented Nov 15, 2023 •
edited

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

This deprecates the className prop, but does not remove it yet. Users should wrap the <Markdown> component inside a <div> manually instead.

Closes #781

@remcohaszing
Deprecate the className prop
ea2fa56 
Users should wrap the `<Markdown>` component inside a `<div>` manually
instead.

Closes #781
@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Nov 15, 2023
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
JounQin
JounQin requested changes Nov 16, 2023
View reviewed changes
Copy link
Member

@JounQin JounQin left a comment

Choose a reason for hiding this comment

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

#781 (comment)

@github-actions github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Nov 16, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2023 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2245c64) 100.00% compared to head (40143b1) 100.00%.
Report is 3 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff            @@
##              main      #799   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines         1354      1337   -17     
  Branches       113       110    -3     
=========================================
- Hits          1354      1337   -17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@remcohaszing
Copy link
Member Author

I misinterpreted the meaning of the word deprecated here originally in a late evening.

I interpret deprecated as planned for change or removal with a warning. In this project deprecated means removed with an error, making deprecations semver major.

@remcohaszing remcohaszing added 🗄 area/interface This affects the public interface 🧑 semver/major This is a change labels Nov 17, 2023
@wooorm
Copy link
Member

wooorm commented Nov 17, 2023

Can you drop the changelog entry? I’ll write the changelog/migration manually when the time is there!
Otherwise, 👍

@remcohaszing
Copy link
Member Author

Sure 👍

It just feeld a bit weird to link a non-existent header from the code.

@wooorm
Copy link
Member

wooorm commented Nov 17, 2023

Ah that’s why, hmm, then I’d do a <!-- To do: add heading for slug deprecate-classname -->
Or these two comments might be enough!

@remcohaszing
Copy link
Member Author

Since this change is breaking, this PR shouldn’t be merged until the next major anyway. I think this PR thread is plenty reminder once this gets merged. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JounQin JounQin JounQin requested changes

@wooorm wooorm wooorm approved these changes

@ChristianMurphy ChristianMurphy ChristianMurphy approved these changes

Assignees
No one assigned
Labels
🗄 area/interface This affects the public interface 🤞 phase/open Post is being triaged manually 🧑 semver/major This is a change
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove the className prop
5 participants
@remcohaszing @codecov-commenter @wooorm @ChristianMurphy @JounQin