[4.0] Remove "bootstrap_size" field by C-Lodder · Pull Request #18957 · joomla/joomla-cms

C-Lodder · 2017-12-02T15:03:53Z

Pull Request for Issue #18866

Summary of Changes

This PR removed every reference to the bootstrap_size field for modules

Quy · 2017-12-02T15:28:39Z

templates/cassiopeia/html/modules.php

-	$moduleTag     = $params->get('module_tag', 'div');
-	$headerTag     = htmlspecialchars($params->get('header_tag', 'h4'));
-	$headerClass   = htmlspecialchars($params->get('header_class', ''));
+	$modulePos	 = $module->position;


Quy · 2017-12-02T15:28:54Z

templates/cassiopeia/html/modules.php

-	$moduleTag     = $params->get('module_tag', 'div');
-	$headerTag     = htmlspecialchars($params->get('header_tag', 'h4'));
-	$headerClass   = htmlspecialchars($params->get('header_class', ''));
+	$modulePos	 = $module->position;


Bakual · 2017-12-02T15:42:20Z

administrator/templates/atum/html/modules.php

 		$headerClass    = ($headerClass) ? ' ' . htmlspecialchars($headerClass) : '';

-		echo '<div class="' . $moduleClass . '">';
+		echo '<div class="col-md-6">';


Does it make sense to hardcode that class here? Wasn't the intent to be able to use the module class (suffix) parameter to set a width? If it's done like this, there will be no way to change the width.

The module class suffix is for styling related classes, which should never be applied to grid related elements.

Incorrect:

.myclass { padding: 100px; margin: 20px; background: #000; } <div class="col-md-6 myclass"> </div>

Correct:

.myclass { padding: 100px; margin: 20px; background: #000; } <div class="col-md-6"> <div class="myclass"></div> </div>

Our aim to deal with the widths for the cpanel modules will come at a later date (future PR)

You don't do any specific for the cpanel modules. You do something for any modules on any position and any templates or it is wrong.
I don't understand why you want to make a difference between grid and design. Both are CSS classes in the end. Btw, you forgot one option in your examples 😄

.myclass > div { padding: 100px; margin: 20px; background: #000; } <div class="col-md-6 myclass"> <div>...</div> </div>

The problem with the current module suffix code is that it is often applied both by the module chrome and the module itself, making the suffix appear twice and thus breaking a lot of CSS. But that is bad design in the module code, not a problem with the parameter itself 😄

I purposely did use the CSS example you provided because the next child element may not be a <div>.

The modules in the FE template don't support the bootstrap widths anymore as the page structure is defined using css-grid.

If you assign 4 modules to the top-a position for example, they will be evenly divided into a 4 column grid. If you want to start adjusting page structures, then you need do this at template level, where it should be done.

Yep, stripping it out is fine. Don't get me wrong. But by hardcoding the class col-md-6 into the module chrome, you hardcode that chrome to always use 50% width. That's what I mean which is wrong. The chrome itself shouldn't apply width (grid) related formatting at all by itself.
That chrome isn't necessary restricted to the cpanel. It may for example as well be used by 3rd parties withing their backend if they use own module positions, or the user may apply the chrome to a module in another position.

So what speaks against removing the hardcoded col.md-6 here and add it to the modules that are shown? Or if you don't want to do that in each module, you can always do what we do today and set it in the cpanel layout as default value.

Like I said, we'll deal with the module widths more appropriately once we've discussed the best solution internally. Of course I don't want to leave the col-md-6 hardcoded in there

There is a PR open for module sizing... #18516

Yep I saw that and it uses the module class parameter like I suggested to do here as well. That's why I'm confused.

Not using bootstrap opens up some options. With no longer such a tight coupling to the markup we can use the module class field for both styling and layout. The downside been we have to custom write the CSS. We already do so with the likes of #18319.

N6REJ · 2017-12-02T21:05:15Z

removing the param I think is the wrong direction. The whole purpose was to give the user flexibility in sizing the module. Now you've not only removed that ability but forced them into a fixed state. i.e. we're back into 1.5 days.

brianteeman · 2017-12-02T21:10:07Z

Please read all the comments @N6REJ

C-Lodder · 2017-12-02T22:05:54Z

@N6REJ The page structure has absolutely no association with Bootstrap. We do NOT use Bootstrap column classes anymore for Cassiopeia. We are utilising CSS Grid which may be a "1.5 days" approach to you, but a 2017 approach to all UI developers

N6REJ · 2017-12-05T23:27:25Z

@C-Lodder you state that put yet you put the column class in!

<div class="col-md-6">
    <div class="myclass"></div>
</div>

I can only go by what you say/show/do.
While the name / function needs to be changed, the PURPOSE of it is valid.
grid does have column size & number ability just like bs does with col- so why not provide that ability EASILY.

mbabker · 2017-12-06T00:04:13Z

The UI options are inherently limited without templates putting in their own parameters. Do we offer params mapping to just col-X or do we offer params for col-sm-X, col-md-X, col-lg-X, and col-xl-X as well? What if the UI framework used by the template doesn't use a markup driven grid like Bootstraps? What if its breakpoints don't align with those of Bootstrap (in terms of number and resolution)?

Long and short, the only reliable way to offer these sizing parameters is to continue to use HTML driven grids. Those have issues and limitations that are addressed by the newer CSS Grid implementations and it has been elected to focus on adapting our technology toward newer and preferred trends, not sticking to inherently broken and limited options.

C-Lodder · 2017-12-06T00:12:04Z

@N6REJ - The backend and frontend templates are 2 completely different things. In the FE template we're using CSS grid for the page structure, whereas in Atum we're still using BS4.

you state that put yet you put the column class in!

As you can see by my actual code changes, I do not use col-md-6 in the Cassiopeia modChrome and clearly state in my comment above that I'm referring to Cassiopeia, not Atum.

ghost · 2017-12-09T09:05:44Z

@C-Lodder can you please give Test Instructions in your first Comment so Tester are sure where to look?

N6REJ · 2017-12-09T15:06:26Z

@mbabker I see what you mean. Is there anyway to allow the flexibility of module size param and non-rigid classes also without resorting to forcing the users to create css sizing?

C-Lodder · 2017-12-09T17:20:36Z

Franz, this is only to remove a parameter. Just a code revoew will do

mbabker · 2017-12-09T17:24:11Z

Is there anyway to allow the flexibility of module size param and non-rigid classes also without resorting to forcing the users to create css sizing?

Templates use the existing plugin events to extend the module edit form then use their extended parameters in the module chrome. An option that has always existed so that there isn't a reliance on trying to map a select list offering a mapping to BS2 spanX classes to an inflexible grid system.

Quy · 2017-12-09T17:58:13Z

Remove here:
https://github.com/C-Lodder/joomla-cms/blob/5d80ce9a7edb64b5582a50076365854eaa3a690c/administrator/templates/system/html/modules.php#L54

N6REJ · 2017-12-10T01:52:59Z

@mbabker

Templates use the existing plugin events to extend the module edit form then use their extended parameters in the module chrome

Anywhere explaining the details / how's of that anywhere?

C-Lodder · 2017-12-16T10:57:43Z

@Quy done

dgrammatiko · 2017-12-16T11:08:03Z

@wilsonge merge?

htmgarcia · 2018-08-08T20:24:01Z

@wilsonge @brianteeman are we removing the Bootstrap size parameter in modules? With the use of CSS Grid make no sense to keep a setting that is bootstrap related.

Remove bootstrap_size field

16e4e70

joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Dec 2, 2017

C-Lodder mentioned this pull request Dec 2, 2017

[4.0] Select Bootstrap size for modules #18866

Closed

Quy reviewed Dec 2, 2017

View reviewed changes

Bakual reviewed Dec 2, 2017

View reviewed changes

ciar4n approved these changes Dec 6, 2017

View reviewed changes

C-Lodder added 2 commits December 16, 2017 10:54

remove last instance of size variable

386854b

Merge remote-tracking branch 'upstream/4.0-dev' into bs-size

04dce57

dgrammatiko approved these changes Dec 16, 2017

View reviewed changes

C-Lodder closed this Feb 2, 2018

C-Lodder deleted the bs-size branch February 10, 2018 11:55

Uh oh!

Comments

Conversation

C-Lodder commented Dec 2, 2017

Summary of Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

C-Lodder Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

C-Lodder Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

N6REJ commented Dec 2, 2017

Uh oh!

brianteeman commented Dec 2, 2017

Uh oh!

C-Lodder commented Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

N6REJ commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbabker commented Dec 6, 2017

Uh oh!

C-Lodder commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 9, 2017

Uh oh!

N6REJ commented Dec 9, 2017

Uh oh!

C-Lodder commented Dec 9, 2017

Uh oh!

mbabker commented Dec 9, 2017

Uh oh!

Quy commented Dec 9, 2017

Uh oh!

N6REJ commented Dec 10, 2017

Uh oh!

C-Lodder commented Dec 16, 2017

Uh oh!

dgrammatiko commented Dec 16, 2017

Uh oh!

htmgarcia commented Aug 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

C-Lodder Dec 2, 2017 •

edited

Loading

C-Lodder Dec 2, 2017 •

edited

Loading

C-Lodder commented Dec 2, 2017 •

edited

Loading

N6REJ commented Dec 5, 2017 •

edited

Loading

C-Lodder commented Dec 6, 2017 •

edited

Loading