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][WIP] BS5 JS #31990

Merged
merged 225 commits into from
Jan 23, 2021
Merged

[4.0][WIP] BS5 JS #31990

merged 225 commits into from
Jan 23, 2021

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Jan 10, 2021

Pull Request for Issue # .

Summary of Changes

This PR changes the JS to Bootstrap V5. Although it's a work in progress after d96ee24 all the components are functional so help by testing and pointing out what is BROKEN will be invaluable. Thanks!

Testing Instructions

Check everything backend/front end

What's broken:

  • [FIXED] Field Modals (the extra buttons for editing/creating new Item in various forms, Modals are mostly ok)
  • Side by Side Editing com Associations
  • The Toolbar dropdown buttons have wrong html the dropdown needs to be the next sibling element to the button (who did this? )
  • [FIXED] Also Multilingual Status broken (empty popup)
  • Frontend Editing
  • Please fix Copy Template, Manage Folders and New File modals under System>SiteTemplates>Cassiopeia Details and Files.
  • Other things that I haven't tested yet

Documentation Changes Required

There is a component that validates what's working at https://github.com/dgrammatiko/bs5/blob/main/BS5Tests.zip This can act as your DEV documentation (also it can act as part of the e2e tests)...

# Conflicts:
#	administrator/components/com_banners/tmpl/tracks/default.php
#	layouts/joomla/toolbar/modal.php
@ghost
Copy link

ghost commented Jan 10, 2021

dgrammatiko thank you

@ceford
Copy link
Contributor

ceford commented Jan 11, 2021

This seems a bit short on explanation to be in the Patchtester!


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

@brianteeman
Copy link
Contributor

It's clearly labelled as a work in progress

@ghost
Copy link

ghost commented Jan 11, 2021

@ceford
image

package.json Outdated
@@ -56,6 +56,7 @@
"short-and-sweet": "^1.0.2",
"skipto": "^2.1.1",
"tinymce": "^5.6.1",
"ttc-bs5": "^1.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll sucker up and ask the question - why your custom build of BS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically I didnt want to swap the bootstrap from 4 to 5 in the main repo. Probably you can have a dependency on the same package with different versions but I have no clue how to do that and it was way easier to work on clean slate than messing around with the rest of the build tools in the main repo. I guess if both JS and CSS are moving to v5 then you can import that and move the building process in the main repo (1 file). FWIW this is just me showcasing that it's not either hard or requiring a month or so to be done no strings attached here (or to put it in another way: I don't mind if the PD closes this PR, if it is already decided to roll with v4, but would be nice to not drag this too long, eg me spending time here instead of finishing the child templates)

@wilsonge
Copy link
Contributor

image
image
image

So I just tried a partial migration of the SCSS given the popularity of the origin discussion - branch here: https://github.com/wilsonge/joomla-cms/tree/bs5

So far it's taken over 2 hours to just get the scss to compile with all the deprecations and I have something looking like the above. It's going to take months to migrate things. Various form CSS classes have changed which means it's not just months of core development work. It's additional months after that for extension developers who have worked to get their extensions beta ready to adapt. It also means it's now or never for the 4.x Joomla series.

This is going to be discussed in the production meeting tomorrow. I think it's very very unlikely that we'll use bs5 though

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jan 11, 2021

It's going to take months to migrate things

@wilsonge just @extend the replaced classes to bring back the dropped ones. Mark them as deprecated and give it a year for both the Core to adjust but also the Devs (or for the backend keep them here tor all the v4 lifetime). Don't patch (for the time being) the markup... That was my take on the issue, not pure or optimal but totally doable, in few days (sorry can't help here I've dropped SCSS for some time now)

EDIT: The JS part can go even if the SCSS is V4, the classes for all the components are the same and the other change is the data attributes but those are not used for styling. There are some limitations, eg the dropdown in v5 supports, left, right etc but for those the styles are missing (these are new features in v5).

@wilsonge
Copy link
Contributor

wilsonge commented Jan 11, 2021

It's not just the custom select classes - that's just what took most the work for the SCSS to compile - as you say for extension devs I can backport. I improved it again just now by removing the prefix BS was adding to the CSS Vars which adds back most colours. I can probably even fix the fact the entire admin now thinks it's in perpetual mobile view without screwing things up super hard.

But all the elements still don't fit padding/margin wise. For example:

image

Nothing that can't be fixed. But doing it on every page all over the CMS and for the extension devs is going to be super time consuming

@dgrammatiko
Copy link
Contributor Author

It's the renaming of the padding, margin, etc. Basically the old classes now reflect to the browsers own default styling (the css is referring to other classnames). Also I think they dropped completely the inline forms (wise choice as it's something that's against any sensible best practices) and all forms are basically inline since the multi column... Good times...

@wilsonge
Copy link
Contributor

I mean it’s the padding and margins things that will screw up most extension devs. Like it’s not forever and ever but it means going back through every page. It also invalidates any templates people have made compatible already

@dgrammatiko
Copy link
Contributor Author

I'll repeat myself, use extend to bring back the missing classes. Thus not breaking anything and people that want BS5 will have it. The extra baggage can be dropped in a given future date (in 2 years maybe there's a BS6 or BS is totally irrelevant, who knows). @ciar4n what do you think about all these?

@ciar4n
Copy link
Contributor

ciar4n commented Jan 11, 2021

Personally, I would sooner put the work in now and make the jump to BS5. I converted the core views to BS4 for J4 originally and I am pretty sure it only took me an hour or two each day for about a week.

BS5 is a very viable excuse to push the release date out a couple of months.

@ciar4n
Copy link
Contributor

ciar4n commented Jan 11, 2021

Extending classes is a bit messy. My preference would be update the views.

@dgrammatiko
Copy link
Contributor Author

My preference would be update the views.

My preference is NO bootstrap at all 🤣

@brianteeman
Copy link
Contributor

isnt the original discussion about bs5 for the site template
everything @wilsonge has done is for the admin

@wilsonge
Copy link
Contributor

wilsonge commented Jan 12, 2021

My stuff covered all 3 clients - installation, admin + site. I just picked screenshots from installation + admin because they looked more broken visually because I can't actually install sample data to show frontend views easily.

The biggest break to my mind right now is two things. First the form group class is removed - they want to use mb-3 or similar going forwards - just utility classes for padding - so all our form styling is way out (possibly fixable with @extends). Second we need to refactor every media query in the system because their scss mixin changed per their docs.

@Magnytu2
Copy link

Hello, to support the project. I think a lot of us think you can't miss the bootstrap 5 switch now. Joomla needs to take a step ahead and not stay in the back of the train. I can understand that technically it is complicated. But strategically, choosing Bootstrap 5 would be great for the future of Joomla. I love Joomla!

@kawshar
Copy link

kawshar commented Jan 12, 2021

+1 for BS5 (Y)

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jan 12, 2021

Almost there:

Bootstrap components done:

  • alert
  • button
  • carousel
  • collapse
  • dropdown
  • modal
  • popover
  • scrollspy
  • tab
  • toast
  • tooltip

Joomla PHP side:

  • toast

Joomla JS modification side:

  • Field Modals
  • Side by Side Editing com Associations
  • Other things that I haven't tested yet

For anyone that wants to actually help you could download the installable from the GitHub PR and then do some testing to figure out what IS NOT WORKING and report back.

PS: There is a component that validates what's working at https://github.com/dgrammatiko/bs5/blob/main/BS5Tests.zip

@ciar4n
Copy link
Contributor

ciar4n commented Jan 12, 2021

The biggest break to my mind right now is two things. First the form group class is removed - they want to use mb-3 or similar going forwards - just utility classes for padding - so all our form styling is way out (possibly fixable with @extends).

The only reason the .form-group class was removed from BS5 was that it did nothing more than add a bottom margin. It was a duplicated class so its removal makes sense. No reason not to add it back in let's say a system.css (presuming that's still a thing)

Second we need to refactor every media query in the system because their scss mixin changed per their docs.

I haven't examined it closely but seems like nothing more than a find n replace. Maybe I'm missing something.

@ciar4n
Copy link
Contributor

ciar4n commented Jan 12, 2021

I honestly don't think the template developers will have an issue here. If they already override everything they likely will continue doing so. If they don't then the changes are minimal

If there is an overall commitment to this then I'll throw my hat in and commit to help converting the core views.

@frostmakk
Copy link
Contributor

Something broken in Joomla Update.
The Pre-Update tab is missing, and Reinstall Joomla core files is missing in Live Update.
Console error: Uncaught ReferenceError: $ is not defined

@wilsonge
Copy link
Contributor

You can ignore joomla update - it’s been broken for a few weeks. It’s unrelated to this PR - but is something I need to look into probably with @dgrammatiko

@frostmakk
Copy link
Contributor

frostmakk commented Jan 23, 2021

But it seems ok in the latest nightly build.
?
Edit:
In the latest nightly the pre-update tab and reinstall core is visible. With this PR it is not, so something is different.

@dgrammatiko
Copy link
Contributor Author

@wilsonge I had a PR for that #18912
I'll clean things up and do a PR

@frostmakk
Copy link
Contributor

Open Options for whatever.
Console error: Uncaught TypeError: items is null
image

- Checks for dispose
- Checks that we have an array with elements
- Add the missing options for scrollspy
@dgrammatiko
Copy link
Contributor Author

Console error: Uncaught TypeError: items is null

Fixed with 43e64a2

Thanks @frostmakk

@astridx
Copy link
Contributor

astridx commented Jan 23, 2021

i just run (after 43e64a2) git pull origin pull/31990/head:4.0-dev__BS5, composer i and npm i.
After that I created an contact. While saving the contact, i get the error

Uncaught TypeError: this._menu is undefined
    show http://localhost/joomla-cms4/media/vendor/bootstrap/js/dropdown.es6.js?dc7caea91fbb3738e28bbb84ba49f1d7:129
    toggle http://localhost/joomla-cms4/media/vendor/bootstrap/js/dropdown.es6.js?dc7caea91fbb3738e28bbb84ba49f1d7:125
    _addEventListeners http://localhost/joomla-cms4/media/vendor/bootstrap/js/dropdown.es6.js?dc7caea91fbb3738e28bbb84ba49f1d7:233
    handler http://localhost/joomla-cms4/media/vendor/bootstrap/js/dom-8eef6b5f.js:355
    addHandler http://localhost/joomla-cms4/media/vendor/bootstrap/js/dom-8eef6b5f.js:449
    on http://localhost/joomla-cms4/media/vendor/bootstrap/js/dom-8eef6b5f.js:477
    _addEventListeners http://localhost/joomla-cms4/media/vendor/bootstrap/js/dropdown.es6.js?dc7caea91fbb3738e28bbb84ba49f1d7:230
    Dropdown http://localhost/joomla-cms4/media/vendor/bootstrap/js/dropdown.es6.js?dc7caea91fbb3738e28bbb84ba49f1d7:93
    Dropdown http://localhost/joomla-cms4/media/vendor/bootstrap/js/dropdown.es6.js?dc7caea91fbb3738e28bbb84ba49f1d7:511
    <anonymous> http://localhost/joomla-cms4/media/vendor/bootstrap/js/dropdown.es6.js?dc7caea91fbb3738e28bbb84ba49f1d7:532
    <anonymous> http://localhost/joomla-cms4/media/vendor/bootstrap/js/dropdown.es6.js?dc7caea91fbb3738e28bbb84ba49f1d7:532
    <anonymous> http://localhost/joomla-cms4/media/vendor/bootstrap/js/dropdown.es6.js?dc7caea91fbb3738e28bbb84ba49f1d7:523

@dgrammatiko
Copy link
Contributor Author

After that I created an contact. While saving the contact, i get the error

@astridx should been fixed with e620922

Thanks

@astridx
Copy link
Contributor

astridx commented Jan 23, 2021

@dgrammatiko

@astridx should been fixed with e620922

Yes, now it is fine. Thanks.

@wilsonge wilsonge merged commit 88c498b into joomla:4.0-dev Jan 23, 2021
@wilsonge wilsonge added this to the Joomla 4.0 milestone Jan 23, 2021
@wilsonge
Copy link
Contributor

I think this is good to go to get testing going. Thankyou very much @dgrammatiko !

@brianteeman
Copy link
Contributor

woohoo - thanks

This was referenced Jan 30, 2021
@dgrammatiko dgrammatiko deleted the 4.0-dev__BS5 branch April 1, 2021 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.