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

[4.0] Upgrade column blog layouts to according CSS classes #31570

Merged
merged 5 commits into from
Jan 23, 2021

Conversation

Harmageddon
Copy link
Contributor

@Harmageddon Harmageddon commented Dec 3, 2020

Pull Request for Issue #27478. This is an alternative approach to #30910.

Summary of Changes

Instead of re-adding the parameters that were removed with #18319, this approach searches for the "column" and "ordering" parameters from J3 and converts the values into the according CSS classes, i.e. columns-X in the case of horizontal ordering, or masonry-X in the case of vertical ordering.

Testing Instructions

As this PR modifies the update mechanism, you need to test it by upgrading a J3 site to J4. Note that you might not be able to update afterwards to higher J4-beta versions on the same site (don't know how this behaves with PR update packages). So I'd recommend to set up a clean, new installation (or a copy from an existing one) only for this PR.

  1. Set up a local 3.10-alpha installation.
  2. Create a set of articles, at least 4, in the same category. They should have different text lengths and at least 4 of them should also be featured.
  3. For each of the three menu item types "Category Blog", "Featured" and "List All Categories", create several menu items that differ in the parameters "Number of Columns" and "Multi-Column Order". Some should be using only one column, some two or three. Some should use the global order, some horizontal and some vertical.
  4. To be able to repeatedly test the upgrade process, you might want to create a backup of files and database at this point.
  5. Upgrade your site to J4 using the update package of this PR.
  • At "Components - Joomla! Update", click on "Options".
  • Update Channel: Custom URL
  • Minimum Stability: Development
  • Custom URL: At the GitHub page of this PR, scroll down to the bottom. Click on "Show all checks". Next to "Download — Prebuilt packages are available for download.", click on "Details". Search for the part "You can test this package as an update by using this custom update server:" and copy the URL from there.

How to test the "List All Categories" menu item type

  • Create a menu item of type "List All Categories"
    • Select a Top Level Category: Root
    • In the tab "Blog Layout", you can configure the parameters "Number of Columns" and "Multi-Column Order".
  • Either create a new category, again with at least 4 articles, that has no own menu item, or unpublish all menu items for the category that you created above in step 2.
  • To test, navigate to frontend, click on your "List All Categories" menu item and then click on the title of your category. The category should now be displayed in blog layout according to the parameters you specified above.

Actual result BEFORE applying this Pull Request

("before applying this PR" = using an unpatched update package, such as the normal beta6 or nightly build)

All blog views are rendered in only one column, all with the same order. The above-mentioned parameters are gone without a trace.

Expected result AFTER applying this Pull Request

The number of columns should stay the same compared to before the upgrade. Depending on whether the according menu item used horizontal or vertical ordering, the articles should now be arranged in a similar manner.

More precisely (with "local" = set in the menu item, "global" = in the configuration of com_content):

Before Upgrade (J3)After Upgrade (J4)
columns (local)order (local)columns (global)order (global)CSS class (local)CSS class (global)
3horizontal2verticalcolumns-3masonry-2
3vertical2verticalmasonry-3masonry-2
3(global)2verticalmasonry-3masonry-2
(global)horizontal2verticalcolumns-2masonry-2
(global)(global)2vertical(global)masonry-2
3horizontal1verticalcolumns-3
(global)(global)2vertical(global)masonry-2
1(global)2vertical(global)masonry-2 [1]

Obviously, this table is not exhaustive because listing all possible combinations would take some time and space. So please feel free to come up with your own combinations!

Documentation Changes Required

Not sure where to document this. We definitely need to document what classes are available for layouts in the Cassiopeia template.

Open Points

  1. I'm not sure how to handle the "categories" view. In J3, it has parameters to control the blog layouts which are displayed when users click on one of the categories. In J4, this tab exists as well, but I don't see a "blog class" field there. Should we add such a field there? (This is the reason, why this PR is created as a draft. After fixing this, the PR should be ready to test)
  2. The masonry layout breaks articles in two if it fits the column layout. This is a different display from J3. And looking at this part, which seems to have no effect at all, I doubt if this behaviour is intended or if it is preferable to avoid breaking articles there.
    &[class^="masonry-"],
    &[class*=" masonry-"] {
    display: block;
    column-gap: $cassiopeia-grid-gutter;
    .blog-item {
    margin-bottom: $cassiopeia-grid-gutter;
    page-break-inside: avoid;
    break-inside: avoid;
    }
    }

    I could add another modifier CSS class that could be used to avoid breaking. In that case, the next question would be whether this class should be included in the conversion from J3 to J4 that is done with this PR. Or we could change this for the masonry layout in general, if breaking inside articles is not intended at all. Comments?

Screenshots

order J3 J4
horizontal blog-horizontal-j3 blog-horizontal-j4
vertical blog-vertical-j3 blog-vertical-j4

@Harmageddon Harmageddon changed the title J4 upgrade layout classes [4.0] Upgrade column blog layouts to according CSS classes Dec 3, 2020
@chmst
Copy link
Contributor

chmst commented Dec 3, 2020

@richard67 please have a look here.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31570.

@brianteeman
Copy link
Contributor

The masonry layout breaks articles in two if it fits the column layout.

I am assuming this is referring to article 4 in the screenshot for cassiopeia vertical ?

I agree that the scss implies the intention was to prevent the column break. If you are saying it doesn't work then wont simply fixing it prevent the need for any extra classes

@Harmageddon
Copy link
Contributor Author

I am assuming this is referring to article 4 in the screenshot for cassiopeia vertical ?

Yes, exactly.

I agree that the scss implies the intention was to prevent the column break. If you are saying it doesn't work then wont simply fixing it prevent the need for any extra classes

I guess so. Maybe @ciar4n or somebody who is more involved with the Cassiopeia template can tell whether the current behaviour is intended or not? I don't want to break anything just because I assume it's a bug.

@brianteeman
Copy link
Contributor

Are you using firefox?

@Harmageddon
Copy link
Contributor Author

Are you using firefox?

Ohh. Yes I am. And in Chromium, the breaking is indeed avoided. Good point!

So this seems to be some weird Firefox thing that it doesn't respect break-inside: avoid for elements that have a display: flex. Too tired now, but I'll do some research tomorrow if this is a known bug or behaviour.

@brianteeman
Copy link
Contributor

It is a known bug which is why I asked ;)

See https://www.smashingmagazine.com/2019/02/css-fragmentation/

@Harmageddon
Copy link
Contributor Author

It is a known bug which is why I asked ;)

See https://www.smashingmagazine.com/2019/02/css-fragmentation/

Hmm, if I'm not missing anything, that article says that Firefox has issues with break-avoid: column, break-after and many other things. But that it should support break-inside: avoid / page-break-inside: avoid, which we are using here.
I filed a bug for Firefox, because I couldn't find any information on this behaviour: https://bugzilla.mozilla.org/show_bug.cgi?id=1680720
So for the moment, I'd treat this as a separate browser issue that we might need to keep in mind, but don't need to fix with this PR.

@Harmageddon
Copy link
Contributor Author

For the "categories" view, I now chose the simplest option, that is, adding the "blog class leading" and "blog class" field to the "Blog Layout" tab of this view.
One question I'm asking myself right now: We have these two fields which include the "useglobal" flag, but we don't have a global option to set the blog class for intro or leading articles. Should we add this to the config of com_content? If we don't, this PR is going to replace global options for the number of columns or the order with local options, i.e. a class specified in the respective menu item.

@chmst
Copy link
Contributor

chmst commented Dec 7, 2020

Looks really good. It resolves the migration problem in an elegant way without needs for extra params in J4.

About the categories view:
I don't remember that categories are displayed in columns in j3 and think that these params in j3 were not used at all and can be ignored.

@ChristineWk
Copy link

ChristineWk commented Dec 8, 2020

About the categories view:
I don't remember that categories are displayed in columns in j3 and think that these params in j3 were not used at all and can be ignored.

Checking something on a 3.10-0-alpha4-dev:

a) Articles Options Blog/Featured Layouts > Multi Column Order: down or across.

b) Menu Item Typ: Category List > Choose a Category > there are no columns.

c) Here I'm not sure:
Menu Item Typ: Featured Articles > Show all featured articles from one or multiple categories in a single or multi column layout.
But it's the same as a)

@Harmageddon
Copy link
Contributor Author

About the categories view:
I don't remember that categories are displayed in columns in j3 and think that these params in j3 were not used at all and can be ignored.

No, they are used indeed. I'm going to update the test instructions with this.
If you have a "List all categories" view, you can choose how a single of these categories should be displayed when you click on one of them (given that the category doesn't have its own menu item). Quoting the hint on that parameter tab:

These options are also used when you select one of the category links, on the first page and/or thereafter, unless they are changed for a specific menu item.

This includes the columns parameters. So if you select to display two columns there, this does not apply directly to the "List all categories" view, but to category views accessible from there.

@ChristineWk This PR treats cases a) and c) (plus d) the menu item type "List all categories", as explained above).

b) Menu Item Typ: Category List > Choose a Category > there are no columns.

Correct, the category list doesn't include columns and thus, needs no changes here.

@N6REJ
Copy link
Contributor

N6REJ commented Dec 10, 2020

Where are you getting masonry from? in bs4 its card-columns. https://getbootstrap.com/docs/4.5/components/card/#card-columns

@chmst
Copy link
Contributor

chmst commented Dec 10, 2020

At the moment it is bootstrap4. I would prefer a pure css solution, but this not necessary in scope of this pr

@Harmageddon
Copy link
Contributor Author

Harmageddon commented Dec 10, 2020

Where are you getting masonry from? in bs4 its card-columns. https://getbootstrap.com/docs/4.5/components/card/#card-columns

Masonry classes were introduced in #18319. But yes, card-columns might be an alternative.

@adj9
Copy link

adj9 commented Dec 12, 2020

I have done the various steps from J! 3.10, but then didn't apply the PR and upgraded to J! 4-dev with Joomla! Update.
After the update I have this screen.
Schermata 2020-12-12 alle 12 26 24

The content has remained the same. About the modules you have to reposition them, but it's part of the upgrade steps J3->J4.

With PR enabled, I can't find the "Number of Columns" and "Order of Multiple Columns" parameters in "Blog Layout".
Screan:
Schermata 2020-12-12 alle 12 51 25

Is npm necessary?

@ChristineWk
Copy link

I have done the various steps from J! 3.10, but then didn't apply the PR and upgraded to J! 4-dev with Joomla! Update.
After the update I have this screen.
Schermata 2020-12-12 alle 12 26 24

(?) #28465
#31561

@Harmageddon
Copy link
Contributor Author

Thank you for testing!

After the update I have this screen.

That's a known issue, see comment by @ChristineWk. Not sure if it's fixed by #31561, but we can ignore it for this PR.

In order to update based on this PR here, you need to use a custom update package. I added some details to the testing instructions. If it's not clear what to do, feel free to ask! :-)

With PR enabled, I can't find the "Number of Columns" and "Order of Multiple Columns" parameters in "Blog Layout".

This PR does not re-introduce the parameters, because in #18319, the choice was made to remove these parameters and use CSS classes instead. This PR adds a step to the update to 4.0 (which is why you need to use the update package of this PR instead of applying the PR after the update). There, the parameters from J3 are converted to the according CSS classes for J4.
So after you updated with an update package from this PR, the field "Blog Class" in your screenshot should contain something, and in frontend, the layout should look similar to that from J3.

Is npm necessary?

No, not for this PR.

@adj9
Copy link

adj9 commented Dec 12, 2020

  • Reset del sito, base 3.9.24-dev
  • Upgrade to 3.10
  • Create 4 articles in a category
  • Create a Blog category
  • Upgrade to 4 from URL custom of PR

The position of the items remains and is correct.
Schermata 2020-12-12 alle 16 58 24

Blog Layout of the BackEnd is this
Schermata 2020-12-12 alle 16 58 46

I did not understand how you act on the parameters "Number of Columns" and "Multi-Column Order".

@adj9
Copy link

adj9 commented Dec 12, 2020

Sorry

The parameter Blog Class works.

@adj9
Copy link

adj9 commented Dec 12, 2020

I have tested this item ✅ successfully on 905d509

With a prev steps, the par Blog Class works. :)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31570.

@ghost
Copy link

ghost commented Dec 13, 2020

I have tested this item ✅ successfully on 905d509


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31570.

@ChristineWk
Copy link

@Harmageddon Could you please wait with merging, as I'm trying to start the update from J 3.10.0.alpha4-dev to J 4.0.0 (with the update package) Thks. I hope it will work :-)


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31570.

@ChristineWk
Copy link

a) Update was successful:

screen shot 2020-12-13 at 15 31 25::

b) Your site has been updated. Your Joomla version is now ‎4.0.0-beta6-dev+pr.31570

c) but: It's broken.
An error has occurred.
0 Call to undefined method Joomla\CMS\Application\AdministratorApplication::isAdmin()

Then I made some checks etc:

There are tables not up to date! > Then Repair button > Update Structure > OK now.

Then I checked via "System" > Templates
Administrator: Atum
Site: Cassiopeia and Protostar (Copy)

Maintenance: Global Check in of:
categories_table
extensions_table
modules_table
Result: 219 items check in.

Templates Style: Changed Cassiopeia to default.

System Dashboard = OK
Home Dashboard = Broken (see above response).
Frontend: No Cassiopeia


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31570.

@chmst
Copy link
Contributor

chmst commented Dec 28, 2020

About the Open Points: My suggestion

  1. Adding both input fields into the categories layout is correct.
  2. We add these classes to config.xml
  3. Question is, which class to use during migration for a vertical "down" ordering. A class "column-x" means that the articles appear "across".
  • masonry-x (bootstrap v4) means ordering "down" but has some disadvantages
  • card-column is the correct and newer implementation for masonry but does not say how many columns.
  • something other like columns-down-x or card-columns-x

I think that users would understand best the masonry-x. Otherwise we must explain our own class. We can use this now as is or override the whole in our scss / layouts.

Would be nice to have mor meanings about that.

@richard67
Copy link
Member

As this PR here is still in draft status, and the alternative PR #30910 has been closed, should the issue #27478 be re-opened and the newer duplicate issue #31801 by the same author be closed? Or vice versa?

@Harmageddon Any plans about when you will mark the PR as ready for review?

@ChristineWk
Copy link

@richard67
My personal opinion is: #27478 should not be re-openend and #31801 should be closed, as both refers to this Draft.

@Harmageddon
Copy link
Contributor Author

@Harmageddon Any plans about when you will mark the PR as ready for review?

I planned to allow for a discussion here regarding the open points (global parameter for Blog Class and the things @chmst pointed out). But if everyone implicitly agrees in silence, I'm going to add the global parameter, resolve the review above, and mark it as ready for review in the next few days. ;-)

@chmst
Copy link
Contributor

chmst commented Jan 2, 2021

@Harmageddon yes, please do so. I will be happy to test it and think that it is the best solution we can get.

@Harmageddon
Copy link
Contributor Author

I added the global parameters for the blog class fields, along with a table in the testing instructions above that indicates which setting should lead to which result.
There is one inconsistency that I found (see last row of the table): If in J3, we set the global number of columns to 2, and the local number of columns for one menu item to 1, the update script now uses "columns-2"/"masonry-2" as the global option, but leaves the local class field empty, as one column would be equivalent to not adding any CSS class. However, if this field is empty, the global value of "columns-2"/"masonry-2" is used.
The only solution I see right now would be to use some nonsense class like "columns-1" in the case of a menu item having only one column specified. Does this make sense?

@Harmageddon Harmageddon marked this pull request as ready for review January 5, 2021 10:58
@ChristineWk
Copy link

I added the global parameters for the blog class fields, along with a table in the testing instructions above that indicates which setting should lead to which result.

Thats works. Thank you!

The only solution I see right now would be to use some nonsense class like "columns-1" in the case of a menu item having only one column specified. Does this make sense?

Hmm. Tested something:

Menus Edit Item: Blog Layout it shows:
Use Global masonry-2 (per default)

a) if you want to delete the: masonry-2. It‘s (was) not possible, it will be changed to: "Use Global (masonry-2)" and becomes inactive.

b) To change that menu to 1 column, you have to enter (overwrite) with e.g: column-1. Thats works.

All other menus remains unchanged (= still showing e.g. masonry-2 Layout) = OK.

@richard67
Copy link
Member

@Harmageddon Could you remove the sentence about draft status at the beginning of the testing instructions? Thanks in advance.

@Harmageddon
Copy link
Contributor Author

@richard67 Oh, sorry. Done! Thank you for the reminder. :-)

@drmenzelit
Copy link
Contributor

I have tested this item ✅ successfully on ef54688

All columns parameters I had set up in J 3.10 were modified in the corresponding class (columns-x or masonry-x) after update to J4


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31570.

@ChristineWk
Copy link

ChristineWk commented Jan 9, 2021

Thanks @drmenzelit for testing.

In principle, the test was also successful for me, but I waited for a comment, because of the topic of column-1, as written above. Ev. to enter manually: column-1 (if desired) was just a suggestion & shouldn't be a problem.

@drmenzelit
Copy link
Contributor

@ChristineWk sorry, I missed your comment...

@drmenzelit
Copy link
Contributor

a) if you want to delete the: masonry-2. It‘s (was) not possible, it will be changed to: "Use Global (masonry-2)" and becomes inactive.

b) To change that menu to 1 column, you have to enter (overwrite) with e.g: column-1. Thats works.

a) That is the same behaviour as in columns parameters in J3, if you don't set a specific number of columns in the menu item, the global configuration will be taken
b) Related to a), I think it is ok to have to change manually to columns-1 if you want to have 1 column ( you have to do the same in J3)

@ChristineWk
Copy link

Thanks Viviana for your clarification/help

b) Related to a), I think it is ok to have to change manually to columns-1 if you want to have 1 column ( you have to do the same in J3)

That's what I meant.

@ChristineWk
Copy link

I have tested this item ✅ successfully on ef54688


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31570.

@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31570.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 9, 2021
@Harmageddon
Copy link
Contributor Author

@ChristineWk @drmenzelit Thank you for testing! :-)
I think I agree with you: People who want this very specific setting of having two columns globally, and one column for a single menu item just have to use the workaround with "column-1", or need to remove the global class and set the "column-2" locally. This should be okay, if we document the available classes somewhere.

@drmenzelit drmenzelit mentioned this pull request Jan 12, 2021
@wilsonge wilsonge merged commit 6e8f744 into joomla:4.0-dev Jan 23, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 23, 2021
@wilsonge
Copy link
Contributor

This makes sense to me. Nice job - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants