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

Button: Add __next40pxDefaultSize to Markdown docs #67540

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

mirka
Copy link
Member

@mirka mirka commented Dec 3, 2024

Part of #65751

What?

Ensures that the Buttons in code snippets in Markdown docs are compliant with the new 40px size, and won't log deprecation errors when we start logging them very soon.

(Due to the large amount of files, code snippets in JSDocs will be addressed separately.)

Why?

It pains me to uglify all these docs but I think it's necessary, given that many will copy & paste the snippets. It's a temporary pain though, since we'll clean these up after a few releases.

cc @WordPress/outreach for awareness, but also let me know if you feel strongly that we shouldn't alter all these docs.

How?

In certain cases where it looks clearly unnecessary to demonstrate with a Button in the code snippet, I changed it to a standard button element for simplicity.

@mirka mirka added the [Type] Developer Documentation Documentation for developers label Dec 3, 2024
@mirka mirka self-assigned this Dec 3, 2024
Copy link

github-actions bot commented Dec 3, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @markhowellsmead.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: markhowellsmead.

Co-authored-by: mirka <[email protected]>
Co-authored-by: ndiego <[email protected]>
Co-authored-by: ryanwelcher <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@mirka mirka requested a review from a team December 3, 2024 20:05
@markhowellsmead
Copy link

Is there no way to make that attribute size-independent? Otherwise, changing it from 40px will require another big review and will break a lot of third-party code.

@ndiego
Copy link
Member

ndiego commented Dec 4, 2024

I think having the __next40pxDefaultSize property in examples is quite confusing, and it's not apparent what it's used for. I would prefer to keep the docs as they are, especially since this is temporary. More people will read the docs than copy and paste the code snippets.

@mirka
Copy link
Member Author

mirka commented Dec 4, 2024

Is there no way to make that attribute size-independent? Otherwise, changing it from 40px will require another big review and will break a lot of third-party code.

@markhowellsmead Could you elaborate on what you mean by "size-independent"? The component will not log a deprecation warning in these cases, for example:

<Button size="small" />
<Button __next40pxDefaultSize size="small" />

@markhowellsmead
Copy link

The use of 40px in the attribute key.

@ryanwelcher
Copy link
Contributor

ryanwelcher commented Dec 4, 2024

I completely appreciate the desire to avoid the deprecations notices here. I run into them myself.

That being said, I tend to agree with @ndiego on this. This a temporary issue, so making these changes now will require revisiting all of the examples again to remove them and is, in my opinion, unnecessary code churn.

The notice that is fired explains the issue well enough for it to be addressed by extenders.

@mirka
Copy link
Member Author

mirka commented Dec 4, 2024

The use of 40px in the attribute key.

@markhowellsmead I see! This pixel value was in fact added intentionally, because given the lifespan of this project, there's a non-negligible chance that another design overhaul will come that needs to update the default to a different value. We even considered the risk of the overarching design plans changing while the size migration is still in progress, which again changes the default value (this actually happened). So the verbosity guards against naming collisions (and overall confusion) when there are multiple design migrations happening in parallel or sequentially.

changing it from 40px will require another big review and will break a lot of third-party code.

I hope it's clear that the point of the prop/warning is to prompt reviews and not break consumer code without warning. Because without it, some consumer usages will break due to layout overflows, etc.

@mirka
Copy link
Member Author

mirka commented Dec 4, 2024

Thanks for your feedback everyone! It's reassuring to know that the Outreach team thinks it's ok to omit from the code snippets. I'll update this PR accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants