8000 fix(MenuToggle): updated borderradius and filter padding by thatblindgeye · Pull Request #6614 · patternfly/patternfly · GitHub
[go: up one dir, main page]
Skip to content

fix(MenuToggle): updated borderradius and filter padding#6614

Merged
mcoker merged 6 commits intopatternfly:v6from
thatblindgeye:iss6536_menuToggleBugs
Jun 11, 2024
Merged

fix(MenuToggle): updated borderradius and filter padding#6614
mcoker merged 6 commits intopatternfly:v6from
thatblindgeye:iss6536_menuToggleBugs

Conversation

@thatblindgeye
Copy link
Contributor
@thatblindgeye thatblindgeye commented May 3, 2024

Closes #6536

@thatblindgeye thatblindgeye requested a review from mcoker May 3, 2024 17:17
@thatblindgeye thatblindgeye linked an issue May 3, 2024 that may be closed by this pull request
Menu toggle bugs #6536
Closed
@patternfly-build
Copy link
Collaborator
patternfly-build commented May 3, 2024 •
edited
Loading

Preview: https://patternfly-pr-6614.surge.sh

A11y report: https://patternfly-pr-6614-a11y.surge.sh

@thatblindgeye
Copy link
Contributor Author
thatblindgeye commented May 3, 2024

@mcoker @andrew-ronaldson This doesn't include any update for the "double border" comment mentioned in the original React issue. I wasn't sure what the consensus was on that. We could remove the native focus outline on the text input group's text input when it's inside of a typeahead, expanded MenuToggle, which should work with how this sort of variant is setup in React for typeaheads (the input itself never loses actual focus). Something along the lines of this:

Screen.Recording.2024-05-03.at.1.38.34.PM.mov

@mcoker mcoker mentioned this pull request May 9, 2024
Merged
@thatblindgeye thatblindgeye force-pushed the iss6536_menuToggleBugs branch from 65486d7 to af44a71 Compare May 14, 2024 12:44
@mattnolting mattnolting self-requested a review May 14, 2024 20:58
andrew-ronaldson
andrew-ronaldson approved these changes May 21, 2024
View reviewed changes
Copy link
Collaborator
@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

I think this is looking good

@srambach
Copy link
Member
srambach commented Jun 4, 2024

I see a little thing when in RTL. I haven't dug in to see what it's coming from.
image

@thatblindgeye thatblindgeye force-pushed the iss6536_menuToggleBugs branch from af44a71 to 12b1e1d Compare June 4, 2024 15:37
@mcoker
Copy link
Contributor
mcoker commented Jun 4, 2024

Discussed offline, the text input group borders were changed in a way that the .pf-m-plain modifier is no longer working as expected. Also noticed that there is no border change on focus. A proposal for an update that could address this is:

  • Move the border from .pf-[v]-text-input-group::before back to .pf-[v]-text-input-group__text::before
  • Remove any references to .pf-[v]-text-input-group__text::after (no longer needed)

2 more nice-to-haves for text input group would be:

  • Add an example for the .pf-m-plain variation
  • Focus styles aren't working correctly. I would probably group some focus styles with the existing :hover styles
    • Something that mimics the v5 styling, maybe something like this? 
      @at-root .#{$text-input-group}__text:focus-within,
&:hover {
  --#{$text-input-group}--BorderColor: var(--#{$text-input-group}--m-hover--BorderColor);
 }

@thatblindgeye
Copy link
Contributor Author
thatblindgeye commented Jun 4, 2024

@mcoker addressed comments above, however:

Focus styles aren't working correctly

If we would want the focus styles to match hover style, we'd probably need to focus-within both the TextInputGroup and MenuToggle (though maybe just for typeahead variant). Using just focus works when the text input element receives focus to change the outer pf-m-c-text-input-group border color, but focusing any utilities in a text input group results in the border color not changing still. Same goes for the typeahead menu toggle.

Could be a question for design. If it's any consolation, while it may look odd to not have the outermost border color change, it also may not be necessary since everything else (should) have its own focus styles.

mcoker
mcoker reviewed Jun 5, 2024
View reviewed changes
Copy link
Contributor
@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

I think we can get rid of ::after in the selector list here

patternfly/src/patternfly/components/TextInputGroup/text-input-group.scss

Line 89 in d523df4

&::after {

@thatblindgeye thatblindgeye force-pushed the iss6536_menuToggleBugs branch from d523df4 to 41c8a31 Compare June 5, 2024 12:44
mattnolting
mattnolting approved these changes Jun 11, 2024
View reviewed changes
Copy link
Collaborator
@mattnolting mattnolting left a comment

Choose a reason for hiding this comment

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

LPTM!

mcoker
mcoker approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor
@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

🫵🪨

@mcoker mcoker merged commit 625dcc9 into patternfly:v6 Jun 11, 2024
@mcoker mcoker mentioned this pull request Jun 11, 2024
Closed
@patternfly-build
Copy link
Collaborator
patternfly-build commented Jun 11, 2024

🎉 This PR is included in version 6.0.0-alpha.158 🎉

The release is available on:

Your semantic-release bot 📦🚀

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jun 21, 2024
@thatblindgeye
fix(MenuToggle): updated borderradius and filter padding (patternfly#…
6944b57 
…6614)
@patternfly-build patternfly-build mentioned this pull request Apr 16, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattnolting mattnolting mattnolting approved these changes

@andrew-ronaldson andrew-ronaldson andrew-ronaldson approved these changes

@mcoker mcoker mcoker approved these changes

Assignees

No one assigned

Labels

released on @alpha

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Menu toggle bugs

6 participants

@thatblindgeye @patternfly-build @srambach @mcoker @mattnolting @andrew-ronaldson
0