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(Select): expose render prop and default value param.s #1781

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

booc0mtaco
Copy link
Contributor

@booc0mtaco booc0mtaco commented Oct 12, 2023

Summary:

  • clean up example using unstyled and styled render props
  • remove custom styling and use FPO block instead
  • add snapshots
  • update existing snapshots

Test Plan:

  • Wrote automated tests
  • CI tests / new tests are not applicable
  • Manually tested my changes, but I want to keep the details secret
  • Manually tested my changes, and here are the details:
    • Create an alpha publish and try out in edu-stack or traject as a sanity check if changes affect build or deploy, or are breaking, such as token changes, widely used component updates, hooks changes, and major dependency upgrades.

@booc0mtaco
Copy link
Contributor Author

Screen.Recording.2023-10-11.at.16.27.08.mov

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #1781 (c66a26b) into next (971f189) will not change coverage.
Report is 1 commits behind head on next.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             next    #1781   +/-   ##
=======================================
  Coverage   92.35%   92.35%           
=======================================
  Files         147      147           
  Lines        2656     2656           
  Branches      711      710    -1     
=======================================
  Hits         2453     2453           
  Misses        187      187           
  Partials       16       16           
Files Coverage Δ
src/components/Select/Select.tsx 92.63% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

github-actions bot commented Oct 12, 2023

size-limit report 📦

Path Size
components 97.11 KB (+0.07% 🔺)
styles 32.96 KB (0%)

@booc0mtaco
Copy link
Contributor Author

Motivation: uncontrolled components allow the form to behave in a way that's more compatible with frameworks using standard HTTP methods for data persistence/submission (e.g., Remix). exposing this option allows for simpler implementation

@booc0mtaco booc0mtaco requested a review from a team October 12, 2023 00:16
Copy link
Contributor

@jeremiah-clothier jeremiah-clothier left a comment

Choose a reason for hiding this comment

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

Need clarity on the Select.ButtonWrapper and how it's used. I don't see if referenced anywhere and it's added as a new export

Comment on lines 380 to 381
Select.Button = SelectTrigger;
Select.ButtonWrapper = SelectButton;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing to me. What's going on here?

select.button = trigger but select.buttonwrapper = button

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yah, not exactly happy with this as-is. It's referenced in this story, and should be used if someone wants to use BOTH the render prop, but also the existing style for Select.

The idea is that you can use <Select.Button>{text}</Select.Button> which will secretly use that wrapper, or

<Select.Button>
{(props} => (
    <span>{props.label}</span> {/* or something similar */}
)}
</Select.Button>

Which will render the "button" as a plain span. If someone wants to use the wrapper, it wasn't previously available, hence the export. This allows the button to be "headless" AND allows folks who want to do something with the render prop to also use the regular styling. Naming suggestions?

I can add documentation to explain this use case, but the way things work with storybook makes documenting the details here a little cumbersome. Will think about it a bit more.

- add in story docs for story example
- clean up example using unstyled and styled render props
- rename internal methods to better define code behavior and use
- remove custom styling and use FPO block instead
- add snapshots
- update existing snapshots
@booc0mtaco booc0mtaco merged commit f21e2b6 into next Oct 12, 2023
8 checks passed
@booc0mtaco booc0mtaco deleted the aholloway/EFI-1371 branch October 12, 2023 18:20
@booc0mtaco booc0mtaco mentioned this pull request Oct 13, 2023
booc0mtaco added a commit that referenced this pull request Oct 13, 2023
## [13.6.0](v13.5.0...v13.6.0) (2023-10-13)


### Features

* **Menu:** allow href and onClick to co-exist on a menu item ([#1779](#1779)) ([971f189](971f189))
* **Popover:** allow focus upon popover panel open ([#1782](#1782)) ([76ddbc6](76ddbc6))


### Bug Fixes

* **Accordion:** handle multi-line text in headers ([#1783](#1783)) ([0b3c3e6](0b3c3e6))
* **Select:** expose render prop and default value param.s ([#1781](#1781)) ([f21e2b6](f21e2b6))
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.

2 participants