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 styling issue in Dismissible boxes #2564

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

gerteck
Copy link
Contributor

@gerteck gerteck commented Jul 16, 2024

Fix styling issue for .dismissible-box .btn-close
where global bootstrap css no longer applied to
component with changed classname

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Fixes #2558

Overview of changes:

Changes in #2487 of class name of body wrapper from alert-dismissible to alert-dismissible-box caused global bootstrap css styles on .alert-dimissible .btn-close to lose effect, causing this bug.

Added back css styles based on bootstrap css to restore original styling.

Testing instructions:

  • Make appropriate changes in your own branch (a few lines diff), markbind serve -d.

Proposed commit message: (wrap lines at 72 characters)

Fix dismissible-box styling issue

Changes in #2487 renamed the class from alert-dismissible to
alert-dismissible-box, causing Bootstrap CSS styles on
.alert-dismissible .btn-close to lose effect.

This commit restores the original styling by adding back
the necessary CSS styles to .alert-dismissible-box .btn-close.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

Fix styling issue for .dismissible-box .btn-close
where global bootstrap css no longer applied to
component with changed classname
@gerteck
Copy link
Contributor Author

gerteck commented Jul 16, 2024

Just a note,

the testcases added previously don't seem to be able detect this styling regression, either for the close button in the modal, nor this close button for the dismissible box.

Additionally, see #2558 comments for more detailed explanation of cause of bug

@damithc damithc requested a review from Tim-Siu July 17, 2024 15:59
Copy link
Contributor

@Tim-Siu Tim-Siu 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 the Pull Request is very clear and brought the disired properties from bootstrap.min.css and alert-dismissible back to the custom wrapper alert-dismissible-box. It solved the bug introduced in #2487.

I checked the issue of concern and it is now fixed. As the added css properties are from bootstrap.min.css and no additional changes have been brought in, I think the PR is safe to merge.

@tlylt
Copy link
Contributor

tlylt commented Jul 23, 2024

Could you help confirm if #2473 is not affected by this change?

As for the test failing to catch this...yes the existing ones may not be able to capture the change in target CSS values for the components, which seems to be the source of many UI positioning related bugs...possibly an area to look into to improve our test capabilities.

@gerteck
Copy link
Contributor Author

gerteck commented Jul 23, 2024

Could you help confirm if #2473 is not affected by this change?

As for the test failing to catch this...yes the existing ones may not be able to capture the change in target CSS values for the components, which seems to be the source of many UI positioning related bugs...possibly an area to look into to improve our test capabilities.

Yeap! Here's a screenshot of my visual test, using minimal reproducible examples:

image

@tlylt
Copy link
Contributor

tlylt commented Jul 23, 2024

Could you help confirm if #2473 is not affected by this change?

As for the test failing to catch this...yes the existing ones may not be able to capture the change in target CSS values for the components, which seems to be the source of many UI positioning related bugs...possibly an area to look into to improve our test capabilities.

Yeap! Here's a screenshot of my visual test, using minimal reproducible examples:

image

Thanks! Will double check later today:)

@tlylt tlylt merged commit c8b304c into MarkBind:master Jul 23, 2024
10 of 11 checks passed
@github-actions github-actions bot added the r.Patch Version resolver: increment by 0.0.1 label Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r.Patch Version resolver: increment by 0.0.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Close button of boxes located too far inside
3 participants