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 bs-modal css variables #2353 #2354

Merged
merged 8 commits into from Aug 24, 2023

Conversation

jmestxr
Copy link
Contributor

@jmestxr jmestxr commented Aug 9, 2023

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 #2353

  • Add .modal class to vue-final-modal component
  • Add display=block for the .modal class to disable display toggling provided by bootstrap
  • Downgrade bootswatch dependency to v5.1.3 in sync with current bootstrap version
  • Add warning about coupling of bootstrap and bootswatch in dev guide

Before:

When using a (non-default) bootswatch theme, bs-modal css variables for modal component are not found, resulting in it not being styled properly:

Screenshot 2023-08-09 at 11 00 26 PM

bootswatch theme used: 'sketchy'


After:

bs-modal CSS variables now registered in modal component:

Screenshot 2023-08-09 at 10 55 44 PM

Anything you'd like to highlight/discuss:

Testing instructions:

  1. update site.json in packages/cli/test/functional/test_site to add any non-default bootswatch theme (see https://markbind.org/userGuide/themes.html)
  2. cd into packages/cli/test/functional/test_site and run markbind serve -d
  3. navigate to test_site/testModals.html in browser and ensure that all modals are rendered correctly in terms of styling

Proposed commit message: (wrap lines at 72 characters)

Register bs-modal css variables in Modal component


Checklist: ☑️

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

@tlylt
Copy link
Contributor

tlylt commented Aug 14, 2023

I completely understand that filling out the PR description can feel like a chore, but it's actually quite valuable for the reviewers. Your effort will go a long way in ensuring a smooth and efficient review process.

Thank you GPT for providing the above 🤣

@tlylt tlylt marked this pull request as draft August 14, 2023 11:45
@jmestxr jmestxr changed the title Fix bs-modal css variables Fix bs-modal css variables #2353 Aug 17, 2023
@jmestxr jmestxr marked this pull request as ready for review August 17, 2023 03:39
@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #2354 (4f09563) into master (9c5db5f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2354   +/-   ##
=======================================
  Coverage   44.82%   44.82%           
=======================================
  Files         122      122           
  Lines        5133     5133           
  Branches     1082     1082           
=======================================
  Hits         2301     2301           
  Misses       2513     2513           
  Partials      319      319           
Files Changed Coverage Δ
packages/vue-components/src/Modal.vue 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tlylt tlylt marked this pull request as draft August 19, 2023 11:04
@tlylt tlylt self-requested a review August 23, 2023 11:58
@tlylt
Copy link
Contributor

tlylt commented Aug 23, 2023

Please add in

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)

To aid review plz. (after you have implemented the changes discussed in the issue)

@jmestxr jmestxr marked this pull request as ready for review August 24, 2023 03:49
Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @jmestxr!

@tlylt tlylt added this to the v5.0.2 milestone Aug 24, 2023
@tlylt tlylt merged commit 677087b into MarkBind:master Aug 24, 2023
8 checks passed
@tlylt
Copy link
Contributor

tlylt commented Aug 24, 2023

@all-contributors please add @jmestxr for code, doc

@allcontributors
Copy link
Contributor

@tlylt

I've put up a pull request to add @jmestxr! 🎉

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.

Modal component: bs-modal css variables not defined
2 participants