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.

Sign up for GitHub

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

Jump to bottom

[BSv5] Adapt cardpane shortcode #1376

Merged
merged 2 commits into from
May 4, 2023
Merged

[BSv5] Adapt cardpane shortcode #1376

merged 2 commits into from
May 4, 2023

Conversation

deining
Copy link
Collaborator

@deining deining commented Jan 25, 2023 •
edited by chalin
Loading

@deining deining added bootstrap shortcodes Hugo shortcodes labels Jan 25, 2023
@deining deining requested a review from chalin January 25, 2023 14:47
chalin
chalin reviewed Jan 25, 2023
View reviewed changes
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Could you add an entry to the CHANGELOG, under breaking-changes so that users are aware of the class name change?

@chalin
Copy link
Collaborator

chalin commented Jan 25, 2023

This PR improves the rendering of card panes, but note that this change still doesn't bring the concerned page in visual agreement with what the paged looked like before.

Before (https://deploy-preview-1356--docsydocs.netlify.app/docs/adding-content/shortcodes/#card-panes):

image

With this PR (https://deploy-preview-1376--docsydocs.netlify.app/docs/adding-content/shortcodes/#card-panes):

image

@deining
Copy link
Collaborator Author

deining commented Jan 25, 2023

Could you add an entry to the CHANGELOG, under breaking-changes so that users are aware of the class name change?

Done.

@deining
Copy link
Collaborator Author

deining commented Jan 25, 2023

This PR improves the rendering of card panes, but note that this change still doesn't bring the concerned page in visual agreement with what the paged looked like before.

I was aware of this when submitting the PR.

From the bootstrap migration guide

BREAKING: Dropped .card-deck in favor of our grid.
Wrap your cards in column classes and add a parent .row-cols-* container
to recreate card decks (but with more control over responsive alignment).

Using of the bootstrap grid in the cardpane shortcode would mean a major rework of the shortcode. I fiddled around with adding a margin/padding to the individual cards, with no satisfactory results. Yes, the page looks different, but it doesn't look bad IMHO. I would leave this as is for now.

@deining deining requested a review from chalin January 25, 2023 15:35
@deining deining force-pushed the issue-1375 branch 2 times, most recently from a6473d2 to cfcbfe1 Compare January 28, 2023 18:53
@deining
Copy link
Collaborator Author

deining commented Jan 28, 2023

This PR improves the rendering of card panes, but note that this change still doesn't bring the concerned page in visual agreement with what the paged looked like before.

On a second attempt, I now was able to bring the concerned page in visual agreement with what the page looked before.
The PR now also closes #1158.

@deining deining force-pushed the issue-1375 branch 2 times, most recently from 0b6d928 to 3e7ba96 Compare February 1, 2023 19:10
chalin
chalin reviewed Feb 2, 2023
View reviewed changes
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

See inline comment.

Also pls rebase, then I'll do a final review.

CHANGELOG.md Outdated Show resolved Hide resolved
@deining
Copy link
Collaborator Author

deining commented Feb 2, 2023

Also pls rebase,

Done.

then I'll do a final review.

Thanks.

@deining deining requested a review from chalin February 2, 2023 21:38
@deining
Copy link
Collaborator Author

deining commented Feb 4, 2023

Following up on #906, I reworked the card shortcode by adding a second commit to this PR. This commit introduces the following changes:

  • shortcode card, card-code: markup of inner content (html/markdown) now depends on the syntax of the calling shortcode, not on extension of page file any more.
  • deprecated shortcode card-code, you can now use shortcode card with named parameter code=true instead.
  • Simplified and improved code of shortcodes.

@deining deining mentioned this pull request Feb 5, 2023
Open
8 tasks
@deining deining force-pushed the issue-1375 branch 3 times, most recently from 8c4f607 to 523b941 Compare February 5, 2023 12:54
chalin
chalin reviewed Feb 13, 2023
View reviewed changes
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Is layouts/shortcodes/card-code.html actually still used? It seems to me that this PR was getting rid of it, but the file is still present.

chalin
chalin reviewed Feb 13, 2023
View reviewed changes
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Other than the question about card-code and border, LGTM. Thanks!

@chalin chalin mentioned this pull request Feb 13, 2023
Closed
50 tasks
@deining
Copy link
Collaborator Author

deining commented Feb 14, 2023

Is layouts/shortcodes/card-code.html actually still used?

No, in the userguide, we don't use it any more.

It seems to me that this PR was getting rid of it, but the file is still present.

CHANGELOG:

- shortcode `card-code` is now deprecated, use shortcode `card` with named
    parameter `code=true` instead.

I wrote deprecated, which means the file is still present and works, but it is not documented any more and users are discouraged to further use this shortcode.

but the file is still present.

Yes, this is intentional: if I remove the shortcode, this will break all existing pages that make use of this shortcode and we will force users to adapt their pages (by replacing shortcode card-code with card).

Does that clarify things? Would you like to see the file deleted?

@deining
Copy link
Collaborator Author

deining commented Feb 14, 2023

Other than the question about card-code and border, LGTM. Thanks!

I rebased the PR to get it ready for merging. Let me know if you want me to delete the card-code shortcode. Styling issues will be addressed in a separate issue, to be opened shortly. Thanks for your thorough review, I appreciate your comments.

@deining deining mentioned this pull request Feb 14, 2023
Open
3 tasks
chalin
chalin reviewed Feb 14, 2023
View reviewed changes
Copy link
Collaborator

@chalin chalin 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 your clarifications. See inline comments.

layouts/shortcodes/card-code.html Show resolved Hide resolved
@deining deining requested a review from chalin February 14, 2023 22:26
chalin
chalin approved these changes May 4, 2023
View reviewed changes
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

I've rebased and resolved the minor conflicts. Thanks for the progressive improvements to this PR.

LGTM ✨

@chalin chalin merged commit e977fe3 into google:main May 4, 2023
5 checks passed
@chalin chalin changed the title Update to bootstrap 5: adapt shortcode 'cardpane' [BSv5] Adapt cardpane shortcode May 4, 2023
@deining deining deleted the issue-1375 branch May 4, 2023 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chalin chalin chalin approved these changes

Assignees
No one assigned
Labels
bootstrap shortcodes Hugo shortcodes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BSv5] Card groups are not rendered properly any more card-code has excess margin at bottom
2 participants
@deining @chalin