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

fix: properly pass this variable context to dynamic list elems #1086

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

Skaiir
Copy link
Contributor

@Skaiir Skaiir commented Mar 6, 2024

Closes #1085

Issue was that restProps also had "value" passed from outer elements, but it was the value of the full dynamic list. So it was overriding the value obtained in the map. Simply changed the variable name to get around the override. Added a test scenario to ensure this never happens again.

@Skaiir Skaiir requested a review from vsgoulart March 6, 2024 14:26
@Skaiir Skaiir self-assigned this Mar 6, 2024
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Mar 6, 2024
@@ -79,11 +79,11 @@ export class RepeatRenderManager {

return (
<>
{displayValues.map((value, index) =>
{displayValues.map((itemValue, itemIndex) =>
<RepetitionScaffold
Copy link
Contributor

Choose a reason for hiding this comment

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

(nitpick, something to keep in mind for the future)

I would have { ...restProps } at the start do that it can't override any explicitly named props in future and help avoid this issue.

React will evaluate properties in order so you can define elements like so

<SomeComponent
  overridableProperty={...}
  {...restProps}
  nonOverridableProperty={...}
/>

Copy link
Contributor

@douglasbouttell-camunda douglasbouttell-camunda left a comment

Choose a reason for hiding this comment

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

Took me a second to understand why this was broken. Good catch! Left you some advice for the future.

@Skaiir Skaiir merged commit fa6e838 into main Mar 6, 2024
13 checks passed
@Skaiir Skaiir deleted the 1085-incorrect-this-assignment-dynlist branch March 6, 2024 15:38
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Mar 6, 2024
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.

3 participants