?

Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
24 Jan 2021

Pull Request for Issue #32116

Summary of Changes

With the introduction of Bootstrap v5, the selector argument in all the methods has changed so it can accept any valid CSS selector. The three methods that create the HTML markup for Tabs, Accordions and Modals are Joomla specific and can continue to function as before, eg: the selector is a word and the method treats it as a class. This PR applies a simple check that if the selector is not a class name will modify it to become one (the old code was doing something similar already).

Testing Instructions

Grab the testing component from https://github.com/dgrammatiko/bs5/blob/main/BS5Tests.zip
Modify the layouts for tab, accordion and modal by removing the . from the second argument of the method.

Actual result BEFORE applying this Pull Request

B/C broken

Expected result AFTER applying this Pull Request

Interactive components work fine

Documentation Changes Required

@wilsonge

avatar dgrammatiko dgrammatiko - open - 24 Jan 2021
avatar dgrammatiko dgrammatiko - change - 24 Jan 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Jan 2021
Category Libraries
avatar laoneo laoneo - test_item - 3 Feb 2021 - Tested successfully
avatar laoneo
laoneo - comment - 3 Feb 2021

I have tested this item successfully on a0ed44c

Tested it with the free version of DPCalendar 8.0.3.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32146.
avatar wilsonge
wilsonge - comment - 5 Feb 2021

Can we be a bit smarter with this? At least check for an id starting with # maybe even not prepend if it starts with data (and assume it’s a data attribute)

avatar richardfrederik67 richardfrederik67 - test_item - 5 Feb 2021 - Tested successfully
avatar richardfrederik67
richardfrederik67 - comment - 5 Feb 2021

I have tested this item successfully on a0ed44c


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

avatar rdeutz
rdeutz - comment - 5 Feb 2021

@richardfrederik67 Please stop creating fake accounts, say you have tested something you haven't tested. You are wasting our time we can use to bring this project forward. You might guessed it, your account is blocked.

avatar richard67 richard67 - alter_testresult - 5 Feb 2021 - richardfrederik67: Not tested
avatar dgrammatiko
dgrammatiko - comment - 5 Feb 2021

Can we be a bit smarter with this? At least check for an id starting with # maybe even not prepend if it starts with data (and assume it’s a data attribute)

@wilsonge I'm not fixing all the selectors for all the methods. This PR affected only the 3 methods that can be called to produce some HTML wrapping text around some existing markup. It might be a good idea to revert to the J3 version of the selectors for these methods. BTW the rest of the functions still can handle any valid CSS selector (which effectivelly is a B/C break)

avatar laoneo
laoneo - comment - 5 Feb 2021

Revert to the J3 version would be nice.

avatar dgrammatiko
dgrammatiko - comment - 5 Feb 2021

Revert to the J3 version would be nice.

I think for the 3 methods (accordion, tab, modal) this is ok, the reasons I didn't did it in this PR were:

  • reverting needed a bit more files touched
  • and I didn't know if this was valid so didn't want to waste my time

But I'm fine doing that if there's an agreement on how to handle (I guess at this point we agree that this is an unneeded break)

avatar wilsonge
wilsonge - comment - 5 Feb 2021

In J3 we do not have a starting #. See https://github.com/Digital-Peak/DPCalendar-Free/blob/main/com_dpcalendar/admin/views/event/tmpl/default.php#L46

I know but there we only accept classes which isn’t ideal either. Ideally we want to transition to allow any Dom selector. So the question is is how to we get there with the best b/c

avatar Fedik
Fedik - comment - 5 Feb 2021

This will not fly.

div.blabla
#foo-bar
custom-element
etc.

Is valid selectors that will be broken

avatar Fedik
Fedik - comment - 6 Feb 2021

I think should be this:

if (strpos($selector, '.') === false && strpos($selector, '#') === false  && strpos($selector, ' ') === false)
{
	$selector = '.' . $selector;
}

Also somewhere need to be noted that "element selector" will not work.
And of course need a "fixed time", when this hack will be removed.

avatar laoneo
laoneo - comment - 6 Feb 2021

At the end if the day we need a solution where we can use the same call in J3 and J4.

avatar HLeithner
HLeithner - comment - 6 Feb 2021

At the end if the day we need a solution where we can use the same call in J3 and J4.

Does it need to be the same call, I mean yes the same call but maybe adding an option for a full css selector in j4, in some of the functions we already have an $options parameter that could be used for it.

Alternative is to use new function names for j4 and write migration functions for j3 which simple always add the "."

avatar dgrammatiko
dgrammatiko - comment - 6 Feb 2021

At the end of the day, we need a solution where we can use the same call in J3 and J4.

The method is the same, the only difference is the second argument is expected to be a real CSS selector in j4 where is J3 was a word interpreted either as class name or id.

Alternative is to use new function names for j4 and write migration functions for j3 which simple always add the "."

something similar is what I had proposed to George when we were discussing the JS PR (eg handle the break in the J3 with a deprecation notice and an additional message in the log). I understand that this opens a can of worms but the current (J3) API will not be sufficient for J4. In J3 you could have called bootstrap.framework to load the JS and then add inline js to initilise any component. With J4 due to all the other changes this isn't needed anymore as long as you call the method for the component (eg HTMLHelper::_('bootstrap.button', 'button[data-something-valid]') will do that without any js from the DEV)

avatar laoneo
laoneo - comment - 7 Feb 2021

Does it need to be the same call, I mean yes the same call but maybe adding an option for a full css selector in j4, in some of the functions we already have an $options parameter that could be used for it.

Alternative is to use new function names for j4 and write migration functions for j3 which simple always add the "."

Yes we need, this is called backwards compatibility. Adding an option for this is ridiculous. @dgrammatiko already made it BC, so we either use it as it is or do look for improvements.

avatar HLeithner
HLeithner - comment - 7 Feb 2021

Yes we need, this is called backwards compatibility. Adding an option for this is ridiculous. @dgrammatiko already made it BC, so we either use it as it is or do look for improvements.

The Option I thought was in j4 only to activate full CSS selector (or the other way around to disable b/c mode)

But I think function with a clean structure and a b/c wrapper would be better

avatar dgrammatiko
dgrammatiko - comment - 7 Feb 2021

I'm not really sure if we're all on the same page here, let me recap:

  • The functions endTab, addTab, endTabSet, startTabSet, renderModal, endSlide, addSlide, endAccordion, startAccordion are Joomla specific, thus we can use the limited way (accepts a word and interprets it as classname or ID). Not great but we're not breaking B/C

  • BUT toast, tooltip, tab, scrollspy, popover, modal, dropdown, collapse, carousel, button, alert are Bootstrap coupled and should be able to accept any valid CSS selector. Also, the implementation rn is not correct, the $selector should default to null and in this case, only the js component is loaded (nothing initialised) and if a $selector is set then also the component will initialise that automatically (no extra js required from the devs in this case). Here the B/C is expected (also kinda impossible to patch it in a way to cover all CSS selectors without writing some spec-compliant CSS selector validator)

avatar Fedik
Fedik - comment - 7 Feb 2021

Selectors is selectors, no mater where they used. Joomla 1.5 used only ID everywhere. Current issue is kind of very old legacy.
I would not do restriction for Tabs/Accordion, but if it will help, well then :)

image

avatar laoneo
laoneo - comment - 8 Feb 2021

So guys, can we not just merge this one and open an issue to find the gold solution. ATM tons of extensions are broken on J4.

avatar wilsonge
wilsonge - comment - 8 Feb 2021

@dgrammatiko can you add the proposed code by Fedik into these accordion/tabs elements so we effectively will accept class/id’s with prefix’s

avatar Fedik
Fedik - comment - 8 Feb 2021

I just noticed that I have missed "attribute selector", here is update:

if (strpos($selector, '.') === false && strpos($selector, '#') === false  
  && strpos($selector, ' ') === false &&  strpos($selector, '[') === false)
{
	$selector = '.' . $selector;
}
avatar dgrammatiko
dgrammatiko - comment - 8 Feb 2021

@wilsonge @laoneo @Fedik I tried another approach: #32357

avatar richard67
richard67 - comment - 8 Feb 2021

@dgrammatiko Does that other PR #32357 replace this one here? Or is it an addition to this here?

avatar dgrammatiko
dgrammatiko - comment - 8 Feb 2021

@richard67 keep both for the time. I have no clue how we want to move forward here

avatar richard67
richard67 - comment - 8 Feb 2021

@dgrammatiko But at the end we chose only 1 of them, right? Just asking to be sure, no problem with it.

avatar dgrammatiko
dgrammatiko - comment - 8 Feb 2021

@richard67 yup at the end there will be one only solution, but it might not be any of these PRs 🤣

avatar laoneo
laoneo - comment - 8 Feb 2021

Can you rebase this branch with 4.0-dev please.

avatar dgrammatiko dgrammatiko - change - 8 Feb 2021
Labels Added: ?
avatar dgrammatiko
dgrammatiko - comment - 10 Feb 2021

This is not quite right according to #32368 (comment) So I'm closing please test #32357

avatar dgrammatiko dgrammatiko - change - 10 Feb 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-02-10 10:43:35
Closed_By dgrammatiko
avatar dgrammatiko dgrammatiko - close - 10 Feb 2021

Add a Comment

Login with GitHub to post a comment