-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/enhanced columns #34
Feature/enhanced columns #34
Conversation
Looking over this, I wonder if this isn't worth contributing back upstream to If they reject the feature request / implementation, I think we can instead incorporate this as |
Makes sense. I'll see if I have the energy to look into their requested process sometime this week. |
If you want, I'm happy to go ahead and file the issue and link to your prototype here - I really do think this is a great improvement over the upstream shortcode and hugely useful for folks! |
That would be very appreciated! |
@binarystargames I think the upstream maintainer is unlikely to merge your contribution to the upstream. Instead of having to maintain our own implementation on top of theirs, can this be reworked to be a new shortcode called |
Yep, I can do that. |
Added a commit that does just that, updated the overview example. I implemented it locally on my site, so that example link is current - that's using the new |
Secondary question: while I'm working on columns, how do you feel about adding a parameter to flatten them whenever the sidebar disappears? We can certainly leave it to CSS to handle too, which is what I'm currently doing for my site. Not sure how you feel about one implementation vs. the other. |
I think having a shortcode parameter like "flattenInMobileView" would be very useful - if people want more control over the presentation, they can ignore that parameter flag and implement their own CSS, but I don't think that's the majority of our target audience. |
Done. To accomplish this, I also added By default it uses default columns behavior, but including |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks great! One minor note unrelated to the code, for this project if we can use rebase to update the working branch instead of merging upstream/main into it, that would be preferable - it helps keep the git history a little neater to follow.
It might also be possible to rebase and combine the first four commits into one - I think this work is really comprised of two chunks: Adding the new shortcode and extending the head injection.
Otherwise the implementation looks great and I'm stoked about this!
Made a few changes as prescribed:
Definitely happy to rebase rather than merge in directly whenever this looks good. |
{{- $platenStyles := resources.Get "platen.scss" | resources.ExecuteAsTemplate "platen.scss" . | resources.ToCSS | resources.Minify | resources.Fingerprint }} | ||
<link rel="stylesheet" href="{{ $platenStyles.RelPermalink }}" {{ template "integrity" $platenStyles }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting here to remind myself:
I have come across this pattern a couple of times now, and I think it's probably worth defining a partial for ourselves so we can reference an SCSS style link injection via something like
{{ partial "platen/GetScssLink" "platen.scss" }}
I've been using some utils
partials over in toroidal - it turns out partials can be used to return values instead of writing text into a file, so that might be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we used a similar value return with a partial with platen/getCssClass
for class injection in baseof
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts, suggestions, and questions. Also worth considering: if there are error conditions, can we handle those explicitly with the errorf
function to give our users (who may not be technical at all) more helpful information than hugo's default when a template breaks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this looks great! I think the only dangling (non-blocking) questions are:
- is if there's any sensible error handling we can use here - maybe to check the ratio string specified, make sure that it's numbers separated by colons?
- can the
flattenInMobileView
parameter be a direct boolean instead of a string?
It's hard to believe, but this is actually pretty robust in that regard. The reason for this is how poorly formed CSS works: At that same Alchemist link, you can see examples of:
The last two broke without a check for ":" so I've included a colon check. In all of the other cases, it just fell back to 1.
Sure can! The following now work:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay! That all makes sense and this looks great! Two minor fixes to the docs and my last (🤞🏻) question: With the way flex-grow
works, I'm wondering if we should warn when people submit an invalid ratio - probably just a single warning message like "Hey this ratio is invalid, we're going to fall back on an even ratio" and maybe include an option to treat the warning as an error.
I couldn't quickly find the docs on turning warnings into errors, but I did find how folks can suppress an error. Not blocking for this PR, I'll merge tonight or tomorrow either way.
d93e1f1
to
c2404d3
Compare
Prior to this commit, theme users were able to specify the `columns` shortcode to arrange their content into multiple evenly distributed columns. This commit builds on that work by introducing a new `flexcolumns` shortcode which allows users to optionally specify a ratio to have more control over column widths.
c2404d3
to
103ef8c
Compare
Added a "ratio" feature to columns shortcode. Code sample/execution:
This code becomes: https://binarystar.games/docs/36th-way/classes/list-of-classes/alchemist/#1st-level
I've also got a CSS rule locally to flatten columns to vertical when hugo-book gets rid of the side TOC/menu because the default hugo-book ones look not great at low width. I left it out for now but it's an option.