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(ui-popover, ui-select): allow overriding Select's dropdown border #1862

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ToMESSKa
Copy link
Contributor

@ToMESSKa ToMESSKa commented Feb 6, 2025

Closes: INSTUI-4395

ISUUE: the border of Select's dropdown cannot be overridden now. CLX team need a version without borders. The border radius need to be changed too, but that can be done with a InstUISettingsProvider.

TEST PLAN:

  • open an example in Select
  • apply following themeoverride on the Select component:
themeOverride={{
    popoverBorderWidth: 'none'
  }}
  • the dropdown should not have a border now
  • it should work with other values too eg. 'large'
  • test the borderRadius too with InstUISettingsProvider

@ToMESSKa ToMESSKa changed the title feat(ui-popover,ui-select): allow overriding Select's dropdown border feat(ui-popover, ui-select): allow overriding Select's dropdown border Feb 6, 2025
@ToMESSKa ToMESSKa self-assigned this Feb 6, 2025
Copy link

github-actions bot commented Feb 6, 2025

PR Preview Action v1.6.0

🚀 View preview at
https://instructure.github.io/instructure-ui/pr-preview/pr-1862/

Built to branch gh-pages at 2025-02-14 10:13 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@ToMESSKa ToMESSKa force-pushed the INSTUI-4395_make_it_possible_to_override_the_border_of_selects_dropdown branch 2 times, most recently from 8775cd7 to 9c06820 Compare February 6, 2025 14:41
@ToMESSKa ToMESSKa requested review from matyasf and balzss February 6, 2025 15:04
*
* Accepts the familiar CSS shorthand to designate border widths corresponding to edges. (e.g. 'none large none large).
*
* Only applies to a Popover without an arrow.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I inserted this about the no arrow version, because the arrow version uses ContextView which would result in having to rewrite ContextView too unfortunately.

@matyasf
Copy link
Collaborator

matyasf commented Feb 10, 2025

So as I see one can now use this prop and after merging #1860 the border radius will work nice too.
After #1860 is merged can you please rebase this and ask the CLX team for a review?

This is the sample code that they need:

<InstUISettingsProvider
  theme={{
        componentOverrides: {
          Select: { popoverBorderWidth: 'large' },
          View: {borderRadiusMedium: '1rem'}
        }
  }}
>
  <SimpleSelect renderLabel="Uncontrolled Select" >
    <SimpleSelect.Option id="foo" value="foo"
                         renderBeforeLabel={(props) => {
                           return <IconCheckSolid />
                         }}>
      Foo
    </SimpleSelect.Option>
    <SimpleSelect.Option id="bar" value="bar">
      Bar
    </SimpleSelect.Option>
    <SimpleSelect.Option id="baz" value="baz">
      Baz
    </SimpleSelect.Option>
  </SimpleSelect>
</InstUISettingsProvider>

Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

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

see my comment above, it good to go from me

@ToMESSKa ToMESSKa force-pushed the INSTUI-4395_make_it_possible_to_override_the_border_of_selects_dropdown branch from bb81af9 to 4fb057c Compare February 14, 2025 10:08
@ToMESSKa ToMESSKa requested a review from igera93 February 14, 2025 10:25
@ToMESSKa
Copy link
Contributor Author

So as I see one can now use this prop and after merging #1860 the border radius will work nice too. After #1860 is merged can you please rebase this and ask the CLX team for a review?

This is the sample code that they need:

<InstUISettingsProvider
  theme={{
        componentOverrides: {
          Select: { popoverBorderWidth: 'large' },
          View: {borderRadiusMedium: '1rem'}
        }
  }}
>
  <SimpleSelect renderLabel="Uncontrolled Select" >
    <SimpleSelect.Option id="foo" value="foo"
                         renderBeforeLabel={(props) => {
                           return <IconCheckSolid />
                         }}>
      Foo
    </SimpleSelect.Option>
    <SimpleSelect.Option id="bar" value="bar">
      Bar
    </SimpleSelect.Option>
    <SimpleSelect.Option id="baz" value="baz">
      Baz
    </SimpleSelect.Option>
  </SimpleSelect>
</InstUISettingsProvider>

@matyasf I rebased it and added @igera93 for review

Copy link
Contributor

@igera93 igera93 left a comment

Choose a reason for hiding this comment

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

Thanks for this change, LGTM 👍

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