pmoreno-rodriguez / grav-theme-future2021

Future Imperfect theme by HTML5UP ported from scratch to Grav. Version 2021
Other
20 stars 10 forks source link

Version 1.0.2 #20

Closed pmoreno-rodriguez closed 1 year ago

pmoreno-rodriguez commented 1 year ago

All original files from version 1.0.2

pmoreno-rodriguez commented 1 year ago

Version 1.0.2

pikim commented 1 year ago

@pmoreno-rodriguez

What's the preferred way to go on? Shall I create pull requests to main branch or to develop branch? If you'd prefer develop branch it should also be reverted to the initial 1.0.2 tag.

pmoreno-rodriguez commented 1 year ago

@pikim, I think it's better to work with the development branch, so we have a chance to test the new features introduced, before releasing them to the main branch.

pikim commented 1 year ago

@pmoreno-rodriguez, OK that's fine. Do you agree that the develop branch should also be reverted to 1.0.2 state first?

pmoreno-rodriguez commented 1 year ago

Hi @pikim, I think the Future2021 theme is ready to continue its development, adding the changes in small pull request and commits. Don't you think? I've been taking a look at the test page you sent me and I quite like it. We could see what new features could be implemented in the new version of the Future2021 theme

pikim commented 1 year ago

Hi @pmoreno-rodriguez, I also wanted to write you this evening as I wanted to continue and commit my fixes. I just compared master, develop and v1.0.2. You recently made some changes to develop, so it's not the same as the other 2 any more. As there are also changes in css files I assume that develop doesn't look identical to v1.0.2 any more. Is this intentional? Shall I just add my fixes to the latest develop or anywhere else? I want to make sure to not mess up everything again.

pmoreno-rodriguez commented 1 year ago

Hi @pikim. Regarding the changes in CSS and SCSS, I wanted to go back a bit, to continue from version 1.0.2 with the new changes. And of course, your collaboration is welcome. Don't feel bad if there are any errors again, the responsibility of the repository is mine too. I think you are much more proficient than me, both Grav and GitHub, so I look forward to your improvements to the theme and we will take small but sure steps.

pikim commented 1 year ago

Hi @pmoreno-rodriguez, meanwhile I committed a lot of changes to my own develop branch. The commits contain bugfixes, improvements, slight modifications and auto-indentation of the twig files. There are almost no changes in css, so that the style of the theme should remain unchanged. How shall we proceed, what do you think?

The css files have to be updated - I didn't do that. Changes in page content and styling are collected in my custom-1.0.2 branch.

pmoreno-rodriguez commented 1 year ago

hi @pikim

I'm going to download your custom 1.0.2 edition and test it on a clean installation of Grav, without additional plugins, with the two predefined pages that Grav installs (home and typography). From there, we will see what errors it shows us, especially when creating a page based on a template that needs a plugin (eg, calendar, etc). I think we should continue working from there, and then we can look at the possibility of introducing new plugin-based templates later.

Note that most Grav users may not want to use a calendar plugin, downloads, guestbook, etc. Why include so many custom and plugin-dependent templates?

pmoreno-rodriguez commented 1 year ago

Also, if we decide to go ahead with including the calendar, events, etc. templates, we should include their dependencies in the theme's blueprints.yaml file. Don't you think?

pmoreno-rodriguez commented 1 year ago

I would not include the 'sidebar' variable in the theme's configuration, but in the blog template's (it makes use of the sidebar variable), since setting it in the theme would affect all blog pages that are defined. Instead, you would define it in the blog template (just like the Quark theme does). What do you think?

pikim commented 1 year ago
  1. please use the develop branch. It doesn't contain the additional templates. The custom-102 branch contains the changes and templates I want on my page in addition to the stock theme. This should also answer your question.
  2. not necessary with develop, see above.
  3. the sidebar is also used in modular and search_results. Any more thoughts?
pmoreno-rodriguez commented 1 year ago

Hi @pikim

I need a few days to catch up with your modifications. I want to count on your support.

pmoreno-rodriguez commented 1 year ago

hi @pikim I'm testing your development version of Future2021. First of all, I want to thank you for all the effort you put into improving the theme. In blog_item.html.twig, I detected that in the div with the "meta" tag, which shows the author name and avatar, and the date of the page, this is not displayed. I think there is a bug in the line that checks if the translate-date plugin is loaded or not. If it is loaded like this, it works, no problem: {% if config.plugins.translate-date.enabled %} (removing brackets and quotes)

Could you confirm that I am correct?

pikim commented 1 year ago

Could you confirm that I am correct?

Yes, you are (almost ;-) ). It must be {% if config.plugins["translate-date"].enabled %} as the minus is not automagically changed into underscore. I fixed this yesterday in commit https://github.com/pikim/grav-theme-future2021/commit/01bc8e5b6dadd0435460669ac99878ff4c9da56b

pmoreno-rodriguez commented 1 year ago

I've just downloaded the latest mods from the develop branch and I'm still having the date issue on blog_item.html.twig and miniposts.html.twig (I've only tested them with these two templates). I still think that it is not necessary to put the square brackets to refer to the name of the plugin, but simply the following:

{% if config.plugins.translate-date.enabled %}

I think neither the brackets nor the quotes are necessary. That's how it works fine. I attached some images so you can see how it looks.

With brackets: miniposts_with_brackets

Without brackets: miniposts_without_brackets

pikim commented 1 year ago

Hmm, that's strange. But is the date really translated in the second image? How does the resulting html look like?

I took the syntax from https://learn.getgrav.org/17/plugins/plugin-basics#accessing-plugin-configuration-values-via-twig as it didn't work "your way" on my setup: the date wasn't translated. Do you use the most recent Grav version?

pmoreno-rodriguez commented 1 year ago

Hi @pikim. I've been using Grav v1.7.38 - Admin v1.10.38. In Grav docs the correct format for plugin that contains dashes is config.plugins['plugin-name'].pluginproperty, with simple quotes, but I've been testing in this last way with the same results. The output HTML is <time class="published" datetime="2023-01-23">Jan 23, 2023</time>.

Well, anyway, I have checked that when the plugin is not installed, the statement {% if config.plugins["translate-date"].enabled %}, is interpreted as if the plugin is installed and activated, so it tries to display the translated date, and since it doesn't exist, nothing is displayed. However, if the plugin is installed and disabled, the above "if" statement works correctly, loading the first condition if the plugin is enabled or the second condition if it is disabled, so it does display a date. However, in my case, in Spanish, the date displayed is still in the English format. I don't know if it's interesting to use this plugin, considering these strange behaviors. What do you think?

pmoreno-rodriguez commented 1 year ago

I've taken a good look at the translate-date plugin's settings and it only has a default setting for en and lt languages, so I don't see it as a plugin that we should spend a lot of time getting into the theme. Many Grav users are not expert programmers and are not going to waste their time translating the months, days and years for their pages. Instead, there's the Twig Extensions plugin, which I've used on occasion, and it does do date conversion without too much effort. I put the following sample code:

{% set lang = grav.language.getActive?: grav.config.site.default_lang %}

{% if config.plugins["twig-extensions"].enabled %}
  <time class="published" itemprop="datePublished" datetime="{{ page.date|localizeddate('medium', 'none', lang) }}">{{ page.date|localizeddate('medium', ' none', lang) }}</time>
{% else %}
  <time class="published" itemprop="datePublished" datetime="{{ page.date|date("Y-m-d") }}">{{ page.date|date("M j, Y") }}</ time>
{% endif %}
pikim commented 1 year ago

Switching to another plugin will not solve this as it is a Grav issue/feature. I just asked the question in the Grav community. Let's see if someone has an answer: https://discourse.getgrav.org/t/how-to-find-out-if-plugin-is-installed-in-twig/22525

pikim commented 1 year ago

Instead, there's the Twig Extensions plugin

... which depends on PHP intl extension which I also wouldn't expect from many non-expert programming Grav users. I chose translate-date because I can easily define long and short month names specifically for each language. Because some may want 2 letter weekdays for German but 3 letter weekdays for another language. And differing lengths for the months. I already had that kind of request for the events plugin.

I can easily implement all that in this plugin. Otherwise I would have to implement a property which contains the desired length for each language and weekday/month. Things become much more complicated then...

pmoreno-rodriguez commented 1 year ago

However, we can decide not to include date translation support through a third-party plugin, simply leave it by default, and let the theme user program multilanguage support for dates. What do you think?

pmoreno-rodriguez commented 1 year ago

Also, if we include the translate-date plugin, it must be mentioned in the theme's README, indicating how to use it.

pikim commented 1 year ago

For me, it's absolutely ok when you don't include it. But, imo, you're too focused on exactly this plugin and you don't think about all the others. If you have a look into the twig files (e.g. sidebar_right.html.twig), you'll see that you'll run into exactly the same problem with them. Checking config.plugins.name.enabled only works if the plugin is installed - it won't work if the plugin itself isn't there, but the according config file is. This is valid for any plugin, not only for translate-date.

See also my thread in the Grav forum and the answer to my question over there.

The same applies for mentioning the plugin(s) in the readme file. All the other plugins aren't mentioned there.

pmoreno-rodriguez commented 1 year ago

Hi @Pikin, you're right, I've been thinking about this issue for too long, but I haven't stopped testing the rest of the files and the theme in a clean installation of Grav. To me, all of your contributions work fine, and I think we could move on to move them to the develop branch of my profile. Once we do some more testing we could move them to the main branch and publish the new version What do you think?

pikim commented 1 year ago

That sounds great to me. I'll continue to collect my personal changes and modifications in my custom-xxx branch. Everything that doesn't affect function and styling will go into my develop branch and hopefully also yours via merge requests.

Just to complete the former topic: if you want be 100% sure that everything works with and without a plugin, you would have to check {% if 'Grav\\Plugin\\TranslateDatePlugin' in grav.plugins|keys and config.plugins["translate-date"].enabled %}. But this would have to be done for every plugin, where TranslateDatePlugin is the class name in the main php file of the plugin.

pmoreno-rodriguez commented 1 year ago

Do it as you think it is easier to implement and maintain, I think that in a matter of plugins (and many other topics) you have more experience than me, so I leave this decision in your hands.

pmoreno-rodriguez commented 1 year ago

What is the purpose of including this line in the blog template?

{% if child.header.hide is not defined or not child.header.hide %}

Where the 'hide' property is defined on 'item' pages. I have tried to put 'hide' in a page type 'item', and I have not seen any difference. What should this line of code do?

pmoreno-rodriguez commented 1 year ago

I'm sorry, the last comment referred to your main branch. I'm testing your develop branch

pikim commented 1 year ago

{% if child.header.hide is not defined or not child.header.hide %}

I'm using the blog template to collect various types of pages and make lists out of them. Sometimes it happens that there is a page that shouldn't be listed, although it resides inside a directory with the blog template. I do this this because the page belongs there logically, but shouldn't be listed anyway.

The property is defined manually in that pages frontmatter. I have no problem doing that manually, as I write all my pages in pure markdown on directory level. I don't use the admin interface for that.

pikim commented 1 year ago

According https://discourse.getgrav.org/t/twig-how-to-find-out-if-plugin-is-installed/22525/4 it is absolutely ok to just check the enabled flag of the plugins. But maybe the plugins should be listed as dependencies.

And, as I said, it's ok for me to remove the translate-date plugin again. By the way: if translations for the desired language don't exist in the plugin, it uses the Grav defaults for the long names. So I think it's no problem that it comes only with a few languages out-of-the-box.

pikim commented 1 year ago

{% if child.header.hide is not defined or not child.header.hide %}

My explanation above was not correct. The property is named page.header.blog_hide_body and I use it on some pages to show only the header in the blog list view, but not the body. I also experienced problems in list view when there's a bigger table on the page.

pmoreno-rodriguez commented 1 year ago

According https://discourse.getgrav.org/t/twig-how-to-find-out-if-plugin-is-installed/22525/4 it is absolutely ok to just check the enabled flag of the plugins. But maybe the plugins should be listed as dependencies.

And, as I said, it's ok for me to remove the translate-date plugin again. By the way: if translations for the desired language don't exist in the plugin, it uses the Grav defaults for the long names. So I think it's no problem that it comes only with a few languages out-of-the-box.

Ok @pikim I have read the Pamtbaau's answer and adding ignore missing is a good option to solve our problem. However, looking for help on another matter, I found this in the Grav docs, https://learn.getgrav.org/17/cookbook/twig-recipes#displaying-a-translated-month I still think we should let each developer add the necessary language plugins to their pages, so we should focus on other aspects of the topic.

pmoreno-rodriguez commented 1 year ago

On the other hand, and more importantly, I am reviewing how the code that refers to the author works, and I have found some bugs:

  1. The author defined in the header of the page or in metadata is not an array, so only one author per page can be defined.
    1. If you click on the link that refers to the author, if it is defined as a taxonomy, it works fine, but if it is defined in the header or metadata of the page, it does not search by author.

This code is inherited from the old Future theme, so I didn't review it in depth, but it's something we should change. I thought of just leaving a code that references the author taxonomy, that would work fine. This has the drawback that the author taxonomy is not predefined in a clean installation of Grav, so the page developer would need to include it for this code to work well.

I'm working on a modification of this code to make it work correctly, although the css styles will probably have to be changed a bit to display both the avatar and the author's name.

pikim commented 1 year ago
  1. The author defined in the header of the page or in metadata is not an array, so only one author per page can be defined.

You can define multiple authors and selecting authors pages using taxonomies works, but only the first author is shown on the list.

  1. If you click on the link that refers to the author, if it is defined as a taxonomy, it works fine, but if it is defined in the header or metadata of the page, it does not search by author.

I don't understand exactly what you mean by this. When the author is set as taxonomy it works fine. But if I understood you correctly you don't want to have the author as subset of the taxonomies, right?

pikim commented 1 year ago

Wouldn't it make sense to simply enter the author under taxonomy to benefit from the builtin functionality instead of making custom modifications?

pmoreno-rodriguez commented 1 year ago

That's what I thought too, so I've made some modifications to remove the author from the page header and metadata, and leave it only in the taxonomies.

pikim commented 1 year ago

In blog_item.html.twig it's sufficient to have

  {% if page.header.author %}
    {% set author = page.header.author %}
  {% else %}
    {% set author = array(page.header.taxonomy.author)[0] %}
  {% endif %}

without further changes. Do you want the ability to have multiple authors with specific avatars and so on?

Eventually the auto author plugin could be modified so that it's able to enter the author as taxonomy.

pmoreno-rodriguez commented 1 year ago

page.header.author and page.header.taxonomy.author not working well with the link to author post in blog item. Take a look to my topic in discourse https://discourse.getgrav.org/t/filtered-author-taxonomy-does-not-work-if-it-is-defined-in-the-page-header/22576 and make some test with two options, the first one with taxonomies defined in site.yaml and the other one with them defined in page header.

pikim commented 1 year ago

I answered you over there. I'm sorry if I didn't understand your question correctly and the answer doesn't help you.

pmoreno-rodriguez commented 1 year ago

Hi @pikim. I have answered in Discourse, with a visual example, so you can get an idea of what I mean. https://discourse.getgrav.org/t/filtered-author-taxonomy-does-not-work-if-it-is-defined-in-the-page-header/22576/3

pmoreno-rodriguez commented 1 year ago

Hi @pikim. I've just modified the blog_item.html.twig in de develop branch.

I think it's better to leave the options to define the author in header and metadata, and filter the author link only if it is defined as taxonomy. If we remove the author definition in the header and metadata from the code, we can remove information from users who are using the theme in an older version. What do you think?

pmoreno-rodriguez commented 1 year ago

I'm still working on improving the theme in other aspects, such as structured data in Schema.org

pikim commented 1 year ago

Of course it is absolutely ok to implement metadata and schema.org. But as I'm no web developer and maintain only one single website I don't know how exactly this will benefit me.

Nevertheless the new version works also works perfectly in my setup. Only one note: in line 32 the span tag has no end.

If you use VScode for development you can use the Twig Language 2 plugin to autoformat Twig code. This helped me multiple times to find such missing tags.

pikim commented 1 year ago

@pmoreno-rodriguez By the way: shall I create a pull request for my changes that don't affect style and functionality? Or did you decide to ignore the changes and fixes?

pmoreno-rodriguez commented 1 year ago

@pmoreno-rodriguez By the way: shall I create a pull request for my changes that don't affect style and functionality? Or did you decide to ignore the changes and fixes?

@pikim, your work is good and I am delighted to be able to count on your help. I answered to your suggestion in https://github.com/pmoreno-rodriguez/grav-theme-future2021/pull/20#issuecomment-1404073206

pmoreno-rodriguez commented 1 year ago

By the way, Did you see this issue? Sounds good. We could to implement in the next version.

pikim commented 1 year ago

@pikim, your work is good and I am delighted to be able to count on your help. I answered to your suggestion in #20 (comment)

OK, I'll create a pull report, then.

By the way, Did you see this issue? Sounds good. We could to implement in the next version.

I saw it. And I like it very much! It would be great to have that in a future release.

pmoreno-rodriguez commented 1 year ago

Hi @pikim How about we include in blog_item.html.twig also support for date translation via Twig Extensions? The code would look like this:


{% set lang = grav.language.getActive ?: grav.config.site.default_lang %}

{% if config.plugins["twig-extensions"].enabled %}
  <time class="published" itemprop="datePublished" datetime="{{ page.date|localizeddate('medium', 'none', lang) }}">{{ page.date|localizeddate('medium', 'none', lang) }}</time>
{% elseif config.plugins["translate-date"].enabled %}
  <time class="published" itemprop="datePublished" datetime="{{ page.date|td(null, "Y-m-d") }}">{{ page.date|td(null, "M j, Y") }}</time>
{% else %}
  <time class="published" itemprop="datePublished" datetime="{{ page.date|date("Y-m-d") }}">{{ page.date|date("M j, Y") }}</time>
{% endif %}
pikim commented 1 year ago

That's fine for me.