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

Simplify fix for abrupt panel transition #2421

Merged
merged 8 commits into from
Feb 17, 2024

Conversation

luminousleek
Copy link
Contributor

@luminousleek luminousleek commented Feb 15, 2024

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

Overview of changes:

Anything you'd like to highlight/discuss:
padding-bottom and margin-bottom are needed to prevent margin collapse

Testing instructions:

NA

Proposed commit message: (wrap lines at 72 characters)

Simplify fix for abrupt panel transition. 

The previous fix implemented a method to compute the max
height of a panel by checking if the collapse button is present
and adding the height of the bottom margin of the button.

Let's simplify this by wrapping the button in a <div> block and 
adding padding to the block. Let's also remove the getMaxHeight
method and its associated tests.

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

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

Hi @luminousleek thanks so much for the work! The solution seems simple and effective. Kind of kicking myself for not doing this in the first place.
One minor nit on the class name and we should be good to do!

:is-open="localExpanded"
@click.native.stop.prevent="toggle(true)"
/>
<div class="card-bottom">
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe bottom-button-wrapper is a better class name? Its similar to what we call our button-wrapper class now :)

Copy link
Contributor

@yucheng11122017 yucheng11122017 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 for the work @luminousleek :))

@yucheng11122017 yucheng11122017 dismissed their stale review February 17, 2024 07:13

missing update test

@yucheng11122017
Copy link
Contributor

Hi @luminousleek could you run npm updatetest to update the expected test cases :)

@@ -17,7 +17,6 @@
<script src="/test_site_algolia_plugin/markbind/js/markbind.min.js"></script>
<script src="index.page-vue-render.js"></script>
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@docsearch/[email protected]/dist/style.css">
<link rel="preconnect" href="https://R2IYF7ETH7-dsn.algolia.net" crossorigin>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line was deleted when i ran npm run updatetest

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you pull from the main branch? I think the change from the latest PR is not reflected thats why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm looks like the deleted line was from the previous PR. the lines added in the algolia.ts file in here are there too. anyhow i've manually added this line back.

Copy link

codecov bot commented Feb 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5ca4704) 48.87% compared to head (4c51140) 48.87%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2421   +/-   ##
=======================================
  Coverage   48.87%   48.87%           
=======================================
  Files         124      124           
  Lines        5238     5238           
  Branches     1109     1109           
=======================================
  Hits         2560     2560           
  Misses       2371     2371           
  Partials      307      307           

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

Copy link
Contributor

@yucheng11122017 yucheng11122017 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 a bunch @luminousleek :)

@yucheng11122017 yucheng11122017 merged commit be70d31 into MarkBind:master Feb 17, 2024
8 checks passed
@yucheng11122017
Copy link
Contributor

@all-contributors please add @luminousleek for code

Copy link
Contributor

@yucheng11122017

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

@yucheng11122017 yucheng11122017 added the r.Patch Version resolver: increment by 0.0.1 label Feb 24, 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.

Simplify fix for panel abruptly transitioning due to bottom panel button
2 participants