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 focus box-shadow for validation stated form-controls #38719

Merged
merged 2 commits into from
Dec 9, 2023

Conversation

KODerFunk
Copy link
Contributor

@KODerFunk KODerFunk commented Jun 6, 2023

Description

Fix focus box-shadow for validation stated form-controls.

Motivation & Context

I use styles with custom build bootstrap with option $enable-shadows: true;. Found a few issues in .form-control styling with classes .is-invalid and .is-valid. In particular, the shadow override for such elements, when in focus, does not use standard wrap that I have used in this PR. Without it, for such elements, in the state of focus, only the outer shadow is exposed.

  1. That is, the first problem is the loss of the inner shadow.
  2. The second problem is the lack of animation, since the transition property only works if number of shadows between states matches.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

Not found.

@KODerFunk KODerFunk requested a review from a team as a code owner June 6, 2023 16:45
@mdo
Copy link
Member

mdo commented Jul 6, 2023

Losing the inner shadow is fine to me, because I think of the "inset" shadow as being lifted once focused. Hence the shadow around the outside. I'd be fine leaving that out still. I also wasn't aware of a loss of transition, haven't seen that. Do you have a demo or screen recording?

@KODerFunk
Copy link
Contributor Author

Hey @mdo! Once again, I will clarify that the code I added is not fiction, it's just a copy-paste of how shadows change in other places. You can search for the comment text Avoid using mixin so we can pass custom focus shadow properly.
The most important thing is the loss of animation. transition does not work for transition from double to single shadow.
If a proof still needs to make a video, I will do it within the next 96 hours.

Copy link
Member

@julien-deramond julien-deramond 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 the PR @KODerFunk. I'm fine with the suggested changes. Before merging it, don't you think we should do the same thing for .form-select and .form-check-input in this same file too?

@KODerFunk
Copy link
Contributor Author

@julien-deramond, sorry for pause and thank you for your understanding. I'll try to take a look at your suggestion and make changes accordingly this weekend.

@KODerFunk
Copy link
Contributor Author

@julien-deramond, @mdo, hi! I recorded several movies. I strengthened inner shadow to make it easier to see problem.

  1. This is how my very first fix works:
fixed-textfield-focus-shadows.mov
  1. Based on a tip from @julien-deramond, I confirm that .form-select has a problem:
broken-select-focus-shadows.mov
  1. Fixed shadows for .form-select in my latest commit:
fixed-select-focus-shadows.mov
  1. .form-check-input and other elements without support for text inside the field do not have shadows when $enable-shadows: true and therefore are not affected by the problem described:
problemless-checkbox-focus-shadow.mov

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @KODerFunk for the PR and the detailed explanations with the videos.

@julien-deramond julien-deramond merged commit 56d80ea into twbs:main Dec 9, 2023
14 checks passed
romankupchak93 pushed a commit to romankupchak93/bootstrap that referenced this pull request Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants