dynamic html tag. html_options{tag:span} Wrapper component in SPAN #480 #1208

tahsin352 · 2019-04-08T17:47:53Z

like react_rails, I have added html_options{tag:'span'}, i.e. dynamic html element property. by default it will use 'div'. Thanks.

This change is

…akacode#480

justin808

Needs unit tests

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tahsin352)

CHANGELOG.md, line 20 at r1 (raw file):

### [11.2.3] - 2019-04-08
#### Improved
- html_options can now has option for 'tag' to add dynamically html element. like this : html_options{tag:'span'}.

html_options supports 'tag' to dynamically set the HTML tag rather than using the default of DIV. html_options{ tag: 'span' }

lib/react_on_rails/helper.rb, line 346 at r1 (raw file):

        content_tag_options.delete("tag")
      else
        content_tag_options_html_tag = div

:div

update to latest

… chromedriver-helper is depricated now. instead we use webdrivers.

justin808 · 2019-04-13T00:40:52Z

CI is failing:


Finished in 37.86 seconds (files took 4.39 seconds to load)
95 examples, 2 failures
Failed examples:
rspec ./spec/helpers/react_on_rails_helper_spec.rb:255 # ReactOnRailsHelper#react_component with 'html_option' tag option should include "<span id=\"App-react-component\"></span>"
rspec ./spec/helpers/react_on_rails_helper_spec.rb:256 # ReactOnRailsHelper#react_component with 'html_option' tag option should script tag be included "<script type=\"application/json\" class=\"js-react-on-rails-component\" data-component-name=\"App\" data-dom-id=\"App-react-component\"></script>\n"
rake aborted!

coveralls · 2019-04-14T04:41:02Z

Coverage remained the same at ?% when pulling da37433 on tahsin352:master into 58bdc34 on shakacode:master.

justin808

Reviewed 1 of 3 files at r2, 3 of 3 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @tahsin352)

CHANGELOG.md, line 20 at r3 (raw file):

### [11.2.3] - 2019-04-08
#### Improved
- html_options can now has option for 'tag' to add dynamically html element. like this : html_options{tag:'span'}.

There's a pattern for the below entries.

lib/react_on_rails/helper.rb, line 342 at r3 (raw file):

    )
      content_tag_options = render_options.html_options
      logger.debug content_tag_options[:tag]

We generally use parens
logger.debug why?
logger.debug { some_expression } won't evaluate the expression unless debug mode.

spec/dummy/spec/helpers/react_on_rails_helper_spec.rb, line 249 at r3 (raw file):

      subject { react_component("App", html_options: { tag: "span" }) }

      it { is_expected.to include "span" }

can we have a test where this option is not present and verify we get div?

justin808

I gave some suggestions.

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @tahsin352)

… default DIV element

justin808

Reviewable status: 3 of 5 files reviewed, 11 unresolved discussions (waiting on @justin808 and @tahsin352)

CHANGELOG.md, line 20 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

There's a pattern for the below entries.

@tahsin352 Please conform to the style of the entries below.

CHANGELOG.md, line 19 at r4 (raw file):

*Please add entries here for your pull requests that are not yet released.*
### [11.2.3] - 2019-04-08
#### Improved

This should be Added, not Improved.

Added means that there is new functionality in that you can use a tag of span.

So we should bump the version to 11.3.0.

Generally, fixed is for X.Y.Z versions where Z changes.

https://semver.org/#spec-item-7

Patch version Z (x.y.Z | x > 0) MUST be incremented if only backwards compatible bug fixes are introduced. A bug fix is defined as an internal change that fixes incorrect behavior.

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes. Patch version MUST be reset to 0 when minor version is incremented.

Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API. It MAY include minor and patch level changes. Patch and minor version MUST be reset to 0 when major version is incremented.

CHANGELOG.md, line 31 at r4 (raw file):

Any truthy values calls hydrate rather than render. (https://github.com/shakacode/react_on_rails/pull/1159) by

this Markdown is incorrect.

CHANGELOG.md, line 42 at r4 (raw file):

  
  Thus, developers will need to fix server rendering errors before continuing.
  [PR 1145](https://github.com/shakacode/react_on_rails/pull/1145) by [justin808](https://github.com/justin808).

@tahsin352 this is the correct markdown

lib/react_on_rails/helper.rb, line 342 at r4 (raw file):

    )
      content_tag_options = render_options.html_options
      if content_tag_options.key?(:tag)

could, should a string option of "tag" be used? rather than a symbol?

I think Ruby converts to a HashWithIndifferentAccess. Can you check?

lib/react_on_rails/helper.rb, line 350 at r4 (raw file):

      content_tag_options[:id] = render_options.dom_id

      rendered_output = content_tag(:"#{content_tag_options_html_tag}",

Please use :to_sym rather than the : operator with an interpolated string

spec/dummy/spec/helpers/react_on_rails_helper_spec.rb, line 255 at r4 (raw file):

      subject { react_component("App") }

      it { is_expected.to include "div" }

I'm pretty sure you'd likely have child elements that are div in both test cases.
We should add a case of not include "span"

Even better, set the id attribute and make sure that element has a tag of either span or div.

…te in rpsec. Change log updated with proper version.

justin808

Reviewed 2 of 2 files at r4, 3 of 3 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tahsin352)

CHANGELOG.md, line 20 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@tahsin352 Please conform to the style of the entries below.

OK

CHANGELOG.md, line 19 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

This should be Added, not Improved.

Added means that there is new functionality in that you can use a tag of span.

So we should bump the version to 11.3.0.

Generally, fixed is for X.Y.Z versions where Z changes.

https://semver.org/#spec-item-7

Patch version Z (x.y.Z | x > 0) MUST be incremented if only backwards compatible bug fixes are introduced. A bug fix is defined as an internal change that fixes incorrect behavior.

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes. Patch version MUST be reset to 0 when minor version is incremented.

Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API. It MAY include minor and patch level changes. Patch and minor version MUST be reset to 0 when major version is incremented.

OK

CHANGELOG.md, line 31 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Any truthy values calls hydrate rather than render. (https://github.com/shakacode/react_on_rails/pull/1159) by

this Markdown is incorrect.

OK -- I can fix this on master

CHANGELOG.md, line 42 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

@tahsin352 this is the correct markdown

OK

lib/react_on_rails/helper.rb, line 342 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

We generally use parens

logger.debug why?

logger.debug { some_expression } won't evaluate the expression unless debug mode.

OK

lib/react_on_rails/helper.rb, line 342 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

could, should a string option of "tag" be used? rather than a symbol?

I think Ruby converts to a HashWithIndifferentAccess. Can you check?

Did you see this question, @tahsin352?

lib/react_on_rails/helper.rb, line 350 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Please use :to_sym rather than the : operator with an interpolated string

OK

spec/dummy/spec/helpers/react_on_rails_helper_spec.rb, line 249 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

can we have a test where this option is not present and verify we get div?

OK

spec/dummy/spec/helpers/react_on_rails_helper_spec.rb, line 255 at r4 (raw file):

Previously, justin808 (Justin Gordon) wrote…

I'm pretty sure you'd likely have child elements that are div in both test cases.
We should add a case of not include "span"

Even better, set the id attribute and make sure that element has a tag of either span or div.

OK

dynamic html tag. html_options{tag:span} Wrapper component in SPAN sh…

cdbcd0e

…akacode#480

tahsin352 mentioned this pull request Apr 8, 2019

Wrapper component in SPAN #480

Closed

justin808 requested changes Apr 9, 2019

View reviewed changes

tahsin352 added 3 commits April 12, 2019 12:22

Merge pull request #1 from shakacode/master

fd25f37

update to latest

rspec BDD test pass for dynamic HTML tag for component shakacode#1208.…

ab130ad

… chromedriver-helper is depricated now. instead we use webdrivers.

rspec BDD for dynamic html_options in react_on_rails_helper spec

150affe

tahsin352 added 3 commits April 13, 2019 23:45

passed rspec helper for travis

bdda2ca

fixed travis ci reported issues like space, single-quote

baefcda

fixed travis ci reported issues like order of webdrivers gem

5fb3eb6

justin808 requested changes Apr 14, 2019

View reviewed changes

justin808 reviewed Apr 14, 2019

View reviewed changes

tahsin352 added 2 commits April 14, 2019 12:33

implemented suggestion on removal of debug log and rspec BDD test for…

9dfe315

… default DIV element

travis ci reported issue regarding trailing whitespace fixed

be9c63d

justin808 requested changes Apr 14, 2019

View reviewed changes

tahsin352 added 3 commits April 22, 2019 16:47

Implemented to_sym in helper. Checking for span / div with ID attribu…

36e1c57

…te in rpsec. Change log updated with proper version.

Random ID added.

de16bb3

have_tag check removed.

da37433

justin808 requested changes Apr 23, 2019

View reviewed changes

justin808 added the enhancement label Apr 23, 2019

justin808 merged commit e6c6d0f into shakacode:master Apr 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dynamic html tag. html_options{tag:span} Wrapper component in SPAN #480 #1208

dynamic html tag. html_options{tag:span} Wrapper component in SPAN #480 #1208

tahsin352 commented Apr 8, 2019 •

edited by justin808

Loading

justin808 left a comment

justin808 commented Apr 13, 2019

coveralls commented Apr 14, 2019 •

edited

Loading

justin808 left a comment

justin808 left a comment

justin808 left a comment

justin808 left a comment

dynamic html tag. html_options{tag:span} Wrapper component in SPAN #480 #1208

dynamic html tag. html_options{tag:span} Wrapper component in SPAN #480 #1208

Conversation

tahsin352 commented Apr 8, 2019 • edited by justin808 Loading

justin808 left a comment

Choose a reason for hiding this comment

justin808 commented Apr 13, 2019

coveralls commented Apr 14, 2019 • edited Loading

justin808 left a comment

Choose a reason for hiding this comment

justin808 left a comment

Choose a reason for hiding this comment

justin808 left a comment

Choose a reason for hiding this comment

justin808 left a comment

Choose a reason for hiding this comment

tahsin352 commented Apr 8, 2019 •

edited by justin808

Loading

coveralls commented Apr 14, 2019 •

edited

Loading