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

Allow pass null argument to methods Tag::class(), Tag::replaceClass(), BooleanInputTag::label() and BooleanInputTag::sideLabel() #78

Merged
merged 9 commits into from
Jul 27, 2021

Conversation

vjik
Copy link
Member

@vjik vjik commented Jul 22, 2021

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Fixed issues -

Also update dev dependencies and fix psalm errors

@vjik vjik added the status:code review The pull request needs review. label Jul 22, 2021
@vjik vjik requested a review from a team July 22, 2021 20:11
@terabytesoftw
Copy link
Member

terabytesoftw commented Jul 24, 2021

Null does not solve the problem, you can still pass an empty string, and if the method is a string, why complicate it, in addition to not being consistent for example:

Example 1:

CustomTag::name('span)->attributes(['class' => 'tests 1')->class('')->render();

expected:

<span class="test 1"></span>

output:

<span class="test 1 "></span>

Example: 2

CustomTag::name('span)->class('')->render();

Expected:

<span></span>

output:

<span class=""></span>

@vjik
Copy link
Member Author

vjik commented Jul 26, 2021

First case is valid:

// Should be <span class="test 1"></span>
CustomTag::name('span)->attributes(['class' => 'tests 1')->class('')->render();

Second case is not valid. I think code CustomTag::name('span')->class('')->render(); should be result <span class=""></span>.

@terabytesoftw
Copy link
Member

Second case is not valid. I think code CustomTag::name('span')->class('')->render(); should be result <span class=""></span>.

What is the point of adding class="" for HTML does not have any meaning makes it <tag class></tag>

@samdark
Copy link
Member

samdark commented Jul 26, 2021

Agree with @terabytesoftw in this case.

@samdark samdark closed this Jul 26, 2021
@samdark samdark deleted the empty-class branch July 26, 2021 12:37
@samdark samdark restored the empty-class branch July 26, 2021 13:06
@samdark samdark reopened this Jul 26, 2021
Copy link
Member

@samdark samdark left a comment

Choose a reason for hiding this comment

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

Discussed once again. It makes sense for consistency sake.

@vjik vjik added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Jul 26, 2021
@samdark
Copy link
Member

samdark commented Jul 26, 2021

Note that this breaks the interface, so needs major release.

@samdark samdark added this to the 2.0.0 milestone Jul 26, 2021
@vjik vjik added status:code review The pull request needs review. and removed status:under development Someone is working on a pull request. labels Jul 27, 2021
@vjik vjik requested a review from samdark July 27, 2021 11:36
@vjik vjik changed the title Allow pass null argument to methods Tag::class() and Tag::replaceClass() Allow pass null argument to methods Tag::class(), Tag::replaceClass(), BooleanInputTag::label() and BooleanInputTag::sideLabel() Jul 27, 2021
@vjik
Copy link
Member Author

vjik commented Jul 27, 2021

BC saved. We add nullable type in final methods of abstract classes. I'm created issue Roave/BackwardCompatibilityCheck#313

@samdark samdark merged commit d32c23a into master Jul 27, 2021
@samdark samdark deleted the empty-class branch July 27, 2021 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review The pull request needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants