Skip to content

Rename Html:add() to Html:addHtml()#111

Merged
dg merged 1 commit intonette:masterfrom
JanTvrdik:pr/html_add
Jun 17, 2016
Merged

Rename Html:add() to Html:addHtml()#111
dg merged 1 commit intonette:masterfrom
JanTvrdik:pr/html_add

Conversation

@JanTvrdik
Copy link
Contributor

@JanTvrdik JanTvrdik commented May 20, 2016

enumag
enumag reviewed May 20, 2016
View reviewed changes
src/Utils/Html.php
* @param Html|string Html node or raw HTML string
* @return self
*/
public function add($child)
Copy link
Contributor

@enumag enumag May 20, 2016

Choose a reason for hiding this comment

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

Maybe only allow Html instance?

Copy link
Contributor

@fprochazka fprochazka May 20, 2016

Choose a reason for hiding this comment

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

Not sure that's a good idea. How would you provide custom HTML as string?

Copy link
Contributor Author

@JanTvrdik JanTvrdik May 20, 2016

Choose a reason for hiding this comment

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

You would need to wrap it as an Html instance. But I think that the current solution is better.

@spaze spaze mentioned this pull request May 20, 2016
Merged
@JanTvrdik
Copy link
Contributor Author

JanTvrdik commented May 20, 2016

Other places which may lead to confusion:

  • $el->insert – maybe not used by anyone directly
  • $el[] – likely used by somebody, don't know how to solve except for deprecating the entire ArrayAccess implementation

milo
milo reviewed May 21, 2016
View reviewed changes
src/Utils/Html.php

/**
* @deprecated
*/
Copy link
Member

@milo milo May 21, 2016

Choose a reason for hiding this comment

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

I like how is the add() short. What about to allow HTML instance and strings consider as a text? Not sure how to warn about change.

@castamir
Copy link

castamir commented May 21, 2016

@JanTvrdik
$el->insert() might became protected or should at least be annotated as @internal
$el[] is a shortcut for adding child elements, isn't it? We can modify array access setter to use addHtml()... or we can deprecate this feature if noone actually use it :D

@dg dg merged commit 1e58e66 into nette:master Jun 17, 2016
dg pushed a commit to nette/forms that referenced this pull request Jun 17, 2016
@spaze @dg
Rename Html:add() to Html:addHtml() to follow nette/utils#111 (#120)
aef9f3a
dg pushed a commit that referenced this pull request Jun 17, 2016
@JanTvrdik @dg
Html: renamed add() to addHtml(); added addText() (#111)
c455ade
dg pushed a commit that referenced this pull request Jun 17, 2016
@JanTvrdik @dg
Html: added addHtml() and addText() (#111)
cdf81ed
dg pushed a commit that referenced this pull request Jun 17, 2016
@JanTvrdik @dg
Html: added addHtml() and addText() (#111)
d372561
dg pushed a commit that referenced this pull request Jun 17, 2016
@JanTvrdik @dg
Html: added addHtml() and addText() (#111)
66ceba1
dg pushed a commit to nette/forms that referenced this pull request Jun 17, 2016
@spaze @dg
Rename Html:add() to Html:addHtml() to follow nette/utils#111 (#120)
a22f01e
@JanTvrdik JanTvrdik deleted the pr/html_add branch June 19, 2016 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@JanTvrdik @castamir @fprochazka @milo @enumag @dg