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

[LOOM-1296][withOpenEvents]: Place openable styling within BpkInput #3408

Merged
merged 2 commits into from
May 2, 2024

Conversation

mungodewar
Copy link
Contributor

@mungodewar mungodewar commented May 2, 2024

Place openable styling within BpkInput.

  • Using data- attribute field we can signal to BpkInput styling that it should be rendered with a cursor: pointer.

Why not use a regular prop?

  • I didn't want to add to the public api of BpkInput (it's really only valid with the withOpenEvents HOC)
  • I didn't want to make a breaking change (to bring the logic of withOpenEvents inside BpkInput) and control that behaviour through props.

An alternative could be:

  • Bring most of the logic inside BpkInput
  • Provide a openable prop to BpkINput
  • withOpenEvents controls just the openable prop
  • BpkInput then controls it's own cursor styling

The above seems like a much larger change, so thought before I'd embark on that to get some feedback on the above first.

Remember to include the following changes:

  • Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g [KOA-123][BpkButton] Updating the colour
  • README.md (If you have created a new component)
  • Component README.md
  • Tests
  • Storybook examples created/updated
  • Type declaration (.d.ts) files updated
  • For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

@mungodewar mungodewar changed the title Loom 1296 [LOOM-1296][withOpenEvents]: Place openable styling within BpkInput May 2, 2024
@mungodewar mungodewar added the patch Patch production bug label May 2, 2024
Copy link

github-actions bot commented May 2, 2024

Warnings
⚠️

Package source files (e.g. packages/package-name/src/Component.tsx) were updated, but type files weren't. Have you checked that no types have changed?

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against 360ae5e

Copy link

github-actions bot commented May 2, 2024

Visit https://backpack.github.io/storybook-prs/3408 to see this build running in a browser.

import STYLES from './BpkInput.module.scss';

const getClassName = cssModules(STYLES);
import { wrapDisplayName } from '../../bpk-react-utils';
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this being used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The wrapDisplayName basically will render something like: withOpenEvents(MyCustomComponent) when viewed in the react debug tools.

It's used when defining the displayName.Without it we'd loose the knowledge that the component had been wrapped when debugging.

Screenshot 2024-05-02 at 10 54 14

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice I see! That's very cool!

Copy link
Member

Choose a reason for hiding this comment

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

We used to use recompose for this but it was deprecated a long time ago so moved to simple code locally as no other replacement was available

@metalix2
Copy link
Contributor

metalix2 commented May 2, 2024

I think it's going to be a challenge for HOC's where they're purpose is to wrap elements, which can then change anything about them. I'm happy with this solution. The data- attribute is an interesting solution. And it does avoid changing the API.

Copy link
Contributor

@metalix2 metalix2 left a comment

Choose a reason for hiding this comment

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

Ship it!

@mungodewar mungodewar merged commit 9ac956c into main May 2, 2024
9 of 10 checks passed
@mungodewar mungodewar deleted the loom-1296 branch May 2, 2024 14:31
KathyWang0208 pushed a commit that referenced this pull request May 27, 2024
…#3408)

* remove className application from withOpenEvents

* update snaps
KathyWang0208 pushed a commit that referenced this pull request May 27, 2024
…#3408)

* remove className application from withOpenEvents

* update snaps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Patch production bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants