-
-
Notifications
You must be signed in to change notification settings - Fork 804
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
[layout] Move layout logic to ruby plugin #2226
Conversation
We enrich the data available to liquid, and avoid complex logic in Liquid this way.
field='eol' | ||
enabled=page.eolColumn | ||
r=r | ||
%} | ||
|
||
{% if page.releaseColumn != false %} | ||
<td {% if diff <= 0 %} class = "txt-linethrough" {% endif %} > <!-- if the support finished add txt-linethrough class --> |
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.
This is currently broken, since diff
is unavailable now.
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.
Looks promising !
columns: | ||
# The spaces before Yes/No | ||
# are intentional to avoid the | ||
# YAML norway problem |
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.
A link to https://www.bram.us/2022/01/11/yaml-the-norway-problem/ could be useful for those who don't know what is the "norway problem" ;)
Do you need help on this one @captn3m0 ? |
I'd like feedback on the schema for the column layouts. For this PR focusing on keeping compatibility, but what will work better for all usecases (including tables in article text)? Setting default values via _config.yml was helpful, but I'm not happy with the code complexity - it should be simpler to write this in Ruby, not harder. |
What do you mean by "the schema for the column layouts" ?
Unfortunately sometimes complexity cannot be avoided. During my work on #2229 I also took a look at the product template. Here are the things I think that make it more complex :
Taken separately those a small things, but it makes it harder to make things more generic (for instance by using |
r['text'][x] = defaults['on-false']['text'] | ||
elsif r[x].is_a? Date | ||
diff = (r[x]-today).to_i | ||
# If EOL is after 6 months |
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.
Current is ~4 months (I used 120 days in #2229).
page['releases'].each do |r| | ||
r['text'] = {} | ||
r['colors'] = {} | ||
['eol', 'support'].each do |x| |
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.
I am not fan of computing colors in the plugin: colors must also be computed for values other than eol
and support
such as discontinued
or custom columns.
IMHO it will be more reusable if it's implemented in a filter such as in #2229.
@captn3m0 should we keep this PR open ? |
1 similar comment
@captn3m0 should we keep this PR open ? |
Closing the PR, as it doesn't seem likely it will be revived. @captn3m0 feel free to drop the branch if there is nothing useful in it. |
We enrich the data available to liquid, and avoid complex logic in Liquid this way.
Still WIP, will make columns a bit more modular, with our sane defaults. Will avoid any mass changes to product front-matter in this PR, and retain compatibility for now. We can take those up in a later PR, since that would include schema changes and require more discussion.
Intent is to use the plugin for any complex logic:
Output of the above (such as rendered templates, description) should be available to the API directly as well.