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

Updated Bootstrap to v5.3 using Bootstrap bundle #85

Open
wants to merge 3 commits into
base: 2.4.x
Choose a base branch
from

Conversation

visto9259
Copy link

Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC no
QA no

Description

  • Updated the Bootstrap assets to Bootstrap v5.3 (CSS and JS). Both the base JS and the bundle JS librairies are available in the /public/js folder.
  • Updated layout.phtml to use the Bootstrap JS bundle library such that is includes Popper which is required by dropdowns.

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @visto9259 - I think that we should use assets directly from the CDN - it makes for fewer files to delete for users that don't wish to use Bootstrap and a cleaner git history.

I'd be in favour of deleting all the existing CSS and JS too if these files become unnecessary.

Perhaps @froschdesign & @Xerkus have more relevant opions about this than me though.

@visto9259
Copy link
Author

Ok.
To my knowledge, there is no CDN that just points to Bootstrap. We will still need to specify the version. If the objective is to prevent updating the skeleton when Bootstrap is updated, then we are not in a better position.

If the objective is to not keep local copies of Bootstrap, then CDN is better.

I will wait for @froschdesign and @Xerkus to comment on this before I make the change to use CDN.

@froschdesign
Copy link
Member

froschdesign commented Mar 31, 2024

The usage from the CDN is good idea because:

  • the user does not have to delete the files when switching to another framework
  • we no longer have to deliver and update the files ourselves

But I have an even more radical suggestion: stop using Bootstrap!
The benefits would be:

  • we can remove the complicated explanations and long view scripts from the Getting Started tutorial; the goal should be <?= $this->form($form) ?> (the usage with Bootstrap can be moved to a cookbook recipe and a notice for TwbsHelper)
  • simple HTML without many classes can be used
  • the layout and the view scripts of the skeleton and tutorial can be independent of a framework

For example, we could use Pico CSS.

@visto9259
Copy link
Author

I already updated the PR to use the CDN.

As far as using Bootstrap or not, I do not believe it is my decision to make since I do not "own" the skeleton.

When I started to use the Zend Framework 3 about 8 or 9 years ago, the skeleton was useful to get me going as I was starting from pretty much nothing in terms of web application. Even Bootstrap was unknown to me (CSS was mysterious concept to me). The fact that it was using Bootstrap got me started with frontend toolkits. Bootstrap is a widely used toolkit. Whether it's good, bad, etc. is a matter of taste. The Getting Started tutorial was also very instructive, even the part on formatting form elements.

If the objective is to get someone going with Laminas MVC, when they already are familiar with HTML/CSS concepts and already have a preferred toolkit, then it should be minimalist in terms of CSS. If the objectve also includes to get going with web development in general (like I was at that time), then using Bootstrap is a good starting point.

The part of the tutorial on updating the class in form elements may seem complicated but one has to know this to render views with the desired look, regardless of the UI toolkit used. It was useful to me when I started.

@froschdesign
Copy link
Member

I already updated the PR to use the CDN.

I have already seen this.

As far as using Bootstrap or not, I do not believe it is my decision to make since I do not "own" the skeleton.

Nobody has asked you to do this. 😄

The Getting Started tutorial was also very instructive, even the part on formatting form elements.

I'm not saying that Bootstrap is bad or that formatting forms isn't important, but this stuff doesn't belong in a getting started tutorial. It is an introduction and also a kind of advertisement for how good and easy it is to use. And that's a horror for a first start, and I don't even want to think about advertising:

<?php
// module/Album/view/album/album/edit.phtml:

$title = 'Edit album';
$this->headTitle($title);
?>
<h1><?= $this->escapeHtml($title) ?></h1>
<?php
$album = $form->get('title');
$album->setAttribute('class', 'form-control');
$album->setAttribute('placeholder', 'Album title');

$artist = $form->get('artist');
$artist->setAttribute('class', 'form-control');
$artist->setAttribute('placeholder', 'Artist');

$submit = $form->get('submit');
$submit->setAttribute('class', 'btn btn-primary');

$form->setAttribute('action', $this->url('album', [
    'action' => 'edit',
    'id'     => $id,
]));
$form->prepare();

echo $this->form()->openTag($form);
?>
<div class="form-group">
    <?= $this->formLabel($album) ?>
    <?= $this->formElement($album) ?>
    <?= $this->formElementErrors()->render($album, ['class' => 'help-block']) ?>
</div>

<div class="form-group">
    <?= $this->formLabel($artist) ?>
    <?= $this->formElement($artist) ?>
    <?= $this->formElementErrors()->render($artist, ['class' => 'help-block']) ?>
</div>

<?php
echo $this->formSubmit($submit);
echo $this->formHidden($form->get('id'));
echo $this->form()->closeTag();

Much better:

<?php
// module/Album/view/album/album/edit.phtml:

$title = 'Edit album';
$this->headTitle($title);
?>
<h1><?= $this->escapeHtml($title) ?></h1>

<?= $this->form($form) ?>

Of course, the links to further explanations must then be added, as we have already done in other tutorials, for example:

And with a cookbook recipe I mean something like this: https://docs.laminas.dev/laminas-mvc-plugin-flashmessenger/cookbook/bootstrap/

Copy link
Member

@froschdesign froschdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I accept this at this point and would defer the decision to drop Bootstrap as this requires further discussion.

@visto9259
Copy link
Author

@gsteel @froschdesign Where do we stand on this PR? I understand that there is no further changes requested or am I missing something?
So that we can close issue #84

@Xerkus
Copy link
Member

Xerkus commented Jul 8, 2024

I am against using CDN in a skeleton. Especially after polyfill cdn supply chain attack.

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

Successfully merging this pull request may close these issues.

4 participants