Skip to content

Support accessor set input#62

Merged
kamilzyla merged 20 commits into
masterfrom
support-accessor-setInput
Jan 27, 2023
Merged

Support accessor set input#62
kamilzyla merged 20 commits into
masterfrom
support-accessor-setInput

Conversation

@averissimo
Copy link
Copy Markdown
Contributor

@averissimo averissimo commented Jan 20, 2023

Related issues

Changes description (include a screenshot if applicable!)

  • Support accessor as well as an index in setInput
  • Adds unit tests in R

Peek 2023-01-20 18-55

Definition of Done checklist

  • 🗒️ README, other documentation and code comments that we have is updated with all information related to the change.

  • Unit tests added for all new or changed logic.

  • End-to-end tests added for all new or changed logic.

@averissimo averissimo marked this pull request as ready for review January 23, 2023 14:26
@averissimo averissimo requested a review from kamilzyla January 23, 2023 14:27
Copy link
Copy Markdown
Collaborator

@kamilzyla kamilzyla left a comment

Choose a reason for hiding this comment

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

Very neat PR!

Comment thread R/react.R Outdated
Comment thread R/react.R Outdated
Comment thread R/react.R Outdated
Comment thread R/react.R Outdated
Comment thread tests/testthat/test-setInput.R
Comment thread js/webpack.config.babel.js Outdated
Comment thread js/src/react/mapReactData.js Outdated
@averissimo averissimo requested a review from kamilzyla January 25, 2023 12:37
@averissimo
Copy link
Copy Markdown
Contributor Author

averissimo commented Jan 25, 2023

I believe I resolved all your comments @kamilzyla

  • removed setInputUsage
  • updated webpack so that there's no hashing option to maintain
  • rename argIdx to jsAccessor
  • break triggerEvent to its own dataMapper called dataMappers.event

Please take a look at the changes I did to setInput as it seemed redundant to have code in R and JS handling the index vs. accessor

I'm a bit ashamed of the time it took to figure out a bug I was having with (...args) to arguments with webpack 😅

Copy link
Copy Markdown
Collaborator

@kamilzyla kamilzyla left a comment

Choose a reason for hiding this comment

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

Great job! Have a bunch more comments, mostly nitpicks about documentation 😉

Comment thread R/react.R Outdated
Comment thread R/react.R Outdated
Comment thread R/react.R Outdated
Comment thread R/react.R Outdated
Comment thread R/react.R Outdated
Comment thread R/react.R Outdated
Comment thread R/react.R Outdated
Comment thread R/react.R Outdated
Comment thread R/react.R Outdated
Comment thread js/src/react/mapReactData.js Outdated
@averissimo
Copy link
Copy Markdown
Contributor Author

I dropped the ball on the documentation. Thank you for catching it!

@averissimo averissimo requested a review from kamilzyla January 26, 2023 18:45
Copy link
Copy Markdown
Collaborator

@kamilzyla kamilzyla left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Great job and thanks for your patience 😃 🎉 Merging with a tiny docs change of mine.

@kamilzyla kamilzyla merged commit 5869b7b into master Jan 27, 2023
@kamilzyla kamilzyla deleted the support-accessor-setInput branch January 27, 2023 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support accessor in setInput()

2 participants