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: handled the validateDOMNesting error #1082

Merged
merged 3 commits into from
Jan 5, 2024

Conversation

0x23d11
Copy link
Contributor

@0x23d11 0x23d11 commented Jan 1, 2024

Description

Please provide a summary of the changes and which issue is fixed. Please include relevant motivation and context.

closes #1065

Changes

There were nesting p elements, I have modified the structure of the elements to get the same output but now the outer element is a div

Screenshots

Before

Screenshot 2024-01-01 at 6 53 26 AM

After

Screenshot 2024-01-01 at 6 54 10 AM

Tests

Please describe how the change has been tested.

@MaxGhenis MaxGhenis requested a review from anth-volk January 1, 2024 02:12
@anth-volk
Copy link
Collaborator

This would fix the issue; however, at the moment, BottomCarousel is very much engineered to accept only text, a clear limitation of the component. Could you change the bottomText prop name to bottomElements or something similar and ensure that the app utilizes that prop properly (I think it's only invoked in two or three spots at the moment)? Otherwise I think it's unclear that that prop is being displayed as a div that other tags can sit within.

Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

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

Please see commentary above

@0x23d11 0x23d11 requested a review from anth-volk January 4, 2024 03:10
@0x23d11
Copy link
Contributor Author

0x23d11 commented Jan 4, 2024

@anth-volk I have updated the code, let me know if there's any correction to be made

Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

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

Thanks again, @dotslashbit. I left one comment that I should've flagged last time and neglected that is meant to keep the functioning of this prop standardized. After that, I think it's good to merge.

@@ -149,7 +149,7 @@ export default function HouseholdOutput(props) {
<BottomCarousel
selected={focus}
options={HOUSEHOLD_OUTPUT_TREE[0].children}
bottomText="PolicyEngine results may not constitute exact tax liabilities or benefit entitlements."
bottomElements="PolicyEngine results may not constitute exact tax liabilities or benefit entitlements."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bottomElements="PolicyEngine results may not constitute exact tax liabilities or benefit entitlements."
bottomElements={<p>PolicyEngine results may not constitute exact tax liabilities or benefit entitlements.</p>}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apologies, should've also mentioned this before in the earlier review.

@0x23d11 0x23d11 requested a review from anth-volk January 5, 2024 01:46
@0x23d11
Copy link
Contributor Author

0x23d11 commented Jan 5, 2024

@anth-volk did the required changes, let me know if there's anything needed to be done

Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

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

This is perfect, thanks so much @dotslashbit!

@anth-volk anth-volk merged commit 8cc7fc2 into PolicyEngine:master Jan 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

validateDOMNesting error within BottomCarousel component
2 participants