Skip to content

Fix imported modal button position off - #2487

Merged
itsyme merged 13 commits into
MarkBind:masterfrom
yiwen101:modal_in_box
Apr 12, 2024
Merged

Fix imported modal button position off#2487
itsyme merged 13 commits into
MarkBind:masterfrom
yiwen101:modal_in_box

Conversation

@yiwen101

@yiwen101yiwen101 commented Mar 30, 2024

Copy link
Copy Markdown
Contributor

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:

Overview of changes:
Fixes#2473
Credit to @tlylt for all the work and investigation; the actual fix code is trivial.

Anything you'd like to highlight/discuss:

Testing instructions:
test with markbind serve -d

Proposed commit message: (wrap lines at 72 characters)
Fix off-positioned close button in imported modal


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).

@yucheng11122017

Copy link
Copy Markdown
Contributor

Hi @yiwen101 I think you shouldn;t be commiting the changes in the binary files

@yiwen101
yiwen101 marked this pull request as draft March 30, 2024 15:23
@yiwen101

Copy link
Copy Markdown
ContributorAuthor

Hi @yiwen101 I think you shouldn;t be commiting the changes in the binary files

Thanks, will revert the result of npm run build:web

@yiwen101
yiwen101 marked this pull request as ready for review March 30, 2024 15:51
@codecov

codecovBot commented Mar 30, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.14%. Comparing base (574e800) to head (7ac9991).

❗ Current head 7ac9991 differs from pull request most recent head 8940f7f. Consider uploading reports for the commit 8940f7f to get more accurate results

Additional details and impacted files
@@ Coverage Diff @@## master #2487 +/- ##
==========================================
+ Coverage 51.00% 51.14% +0.14% 
==========================================
Files 124 124 Lines 5384 5359 -25 Branches 1162 1152 -10 ==========================================
- Hits 2746 2741 -5 + Misses 2348 2328 -20 
Partials 290 290 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yucheng11122017yucheng11122017 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add a functional test for testing the behaviour described in the issue? If you can do a unit test, would be even better but might be a bit tough

@yucheng11122017
yucheng11122017force-pushed the master branch 2 times, most recently from cb84513 to 69ec838CompareApril 5, 2024 06:21
@yiwen101

Copy link
Copy Markdown
ContributorAuthor

Could you add a functional test for testing the behaviour described in the issue? If you can do a unit test, would be even better but might be a bit tough

Sorry for the late reply. I have added the functional test.
To verify the result of fix, remember to do "markbind serve -d", otherwise the position will still be off(since frontend is updated per release).

@EltonGohJHEltonGohJH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

The language for commit message seems off.
how bout Fix off-positioned close button in imported modal

@yiwen101

Copy link
Copy Markdown
ContributorAuthor

LGTM!

The language for commit message seems off. how bout Fix off-positioned close button in imported modal

Thanks for the review and suggestions on the commitment message. Yes, the suggested commit message is clearer. I have also updated the "proposed commit message" for this PR accordingly.

@itsymeitsyme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks for the effort to write the functional tests as well!

@itsyme
itsyme merged commit ff8e9b1 into MarkBind:masterApr 12, 2024
@github-actionsgithub-actionsBot added the r.Patch Version resolver: increment by 0.0.1 label Apr 12, 2024
Tim-Siu pushed a commit to Tim-Siu/markbind that referenced this pull request Apr 12, 2024
---------
Co-authored-by: Chan Yu Cheng <77204346+yucheng11122017@users.noreply.github.com>
@gerteckgerteck mentioned this pull request Jul 16, 2024
14 tasks
tlylt pushed a commit that referenced this pull request Jul 23, 2024
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.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

r.PatchVersion resolver: increment by 0.0.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modals: The x button position is off

4 participants

@yiwen101@yucheng11122017@itsyme@EltonGohJH