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

feat(TearsheetShell): specify additional floating menu selectors #5092

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

liunate
Copy link

@liunate liunate commented May 7, 2024

Contributes to #4985

Allow <TearsheetShell> to focus on elements matching CSS selectors specified

What did you change?

Pass thru and join <TearsheetShell selectorsFloatingMenus> value to the underlying carbon <ComposedModal selectosFloatingMenus>

How did you test and verify your work?

I add the selectorsFloatingMenus property on the one of the story render property in Tearsheet.stories.jsx such like:

      <div ref={ref}>
        <Tearsheet
          selectorsFloatingMenus={['#helloMan']}
        >
          {mainContent}
        </Tearsheet>
        <div id="helloMan" style={{position: 'fixed', left: '16rem', top: '16rem', zIndex: 9999, backgroundColor: 'green'}}>
            <h1> hello </h1>
            <input type='text' defaultValue="abc" />
          </div>
      </div>

Without the #helloMan being specified, the opened tearshet does not allow focusing on the input. When the selector is specified, the input can be focused and type inside.

@liunate liunate requested a review from a team as a code owner May 7, 2024 14:24
Copy link
Contributor

github-actions bot commented May 7, 2024

DCO Assistant Lite bot All contributors have signed the DCO.

Copy link

netlify bot commented May 7, 2024

Deploy Preview for carbon-for-ibm-products ready!

Name Link
🔨 Latest commit 36d3823
🔍 Latest deploy log https://app.netlify.com/sites/carbon-for-ibm-products/deploys/6659221cb4c9d30008a28bf0
😎 Deploy Preview https://deploy-preview-5092--carbon-for-ibm-products.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@liunate
Copy link
Author

liunate commented May 7, 2024

I have read the DCO document and I hereby sign the DCO.

@makafsal
Copy link
Contributor

makafsal commented May 8, 2024

@liunate Code looks fine to me. But could you please add a storybook for this to demonstrate the feature?

@liunate liunate requested a review from a team as a code owner May 19, 2024 08:33
@liunate liunate requested review from elycheea and IgnacioBecerra and removed request for a team May 19, 2024 08:33
Comment on lines +108 to +109
{`${beenOpen ? 'Reopen' : 'Open'} the ${context.component.displayName}`}
</Button>
Copy link
Author

Choose a reason for hiding this comment

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

I think these part could be rewritten in tidier way like this.

Copy link
Author

@liunate liunate May 19, 2024

Choose a reason for hiding this comment

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

context.component.componentName is no longer valid, so I replaced it to show the correct component name. In this case here, the TearsheetShell

);
}

export const FocusOnFloatingMenu = ({ open: _open, ...args }, context) => {
Copy link
Author

Choose a reason for hiding this comment

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

Here is the recording showcasing the new property in action.

allow-floating-story.mov

Copy link
Author

Choose a reason for hiding this comment

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

It works fine in story docs as well

@liunate
Copy link
Author

liunate commented May 22, 2024

Not sure why it failed at the ci-check:test:c4p which succeeded in my local env. Any idea?

@matthewgallo
Copy link
Member

Looks like something is timing out. I just triggered it to run again

@matthewgallo
Copy link
Member

matthewgallo commented May 23, 2024

Hey @liunate, you just have one conflict to resolve in TearsheetShell.stories.jsx.

@@ -179,6 +181,75 @@ NoAttributesSet.args = {
};

export const ReturnFocusToOpenButton = ReturnFocusTemplate.bind({});
NoAttributesSet.args = {
ReturnFocusToOpenButton.args = {
Copy link
Author

@liunate liunate May 25, 2024

Choose a reason for hiding this comment

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

I think this story template specifying the .args was wrong, so I just updated this to ReturnFocusToOpenButton.

@liunate
Copy link
Author

liunate commented May 31, 2024

I just have this PR branch synced again with main, see if the changes now looks good for you?

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.

None yet

4 participants