Skip to content

[5.1] Guided Tours - Add checkbox/radio/select target element support, enhanced required field handling #40994

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

Merged
merged 144 commits into from
Feb 19, 2024

Conversation

GeraintEdwards
Copy link
Contributor

@GeraintEdwards GeraintEdwards commented Jun 21, 2023

Pull Request for Issue # .

Summary of Changes

  • Add checkbox/radio/select target element support for interactive field types
  • Add support for textarea target element alongside input text type targets
  • Allow fields that are not 'required' in the DOM to be required in the field e.g. for tours relating to searching where the input field is required for the tour but not by Joomla or the active page component
  • Allow the tour to insist on a specific required value (translatable) so that the context of the tour can be maintained as it progresses e.g. searching for a specific text value.

Please note that a new database field has been added in this PR 'params' in the guided tours steps table.

Testing Instructions

Test the provided tour.
Check that old tours still function properly.

Actual result BEFORE applying this Pull Request

The tour will not respond to the checkbox, select list and textarea field changes correctly

Expected result AFTER applying this Pull Request

See the tour as it plays out in these screenshots

Screenshot from 2023-06-21 10-25-08

Title: Global config demo tour
URL: /administrator/index.php?option=com_config
Description:

<p>This tour will demonstrate:</p>
<ul>
<li>checkboxes</li>
<li>radio boxes</li>
<li>required text fields searching for specific value</li>
</ul>

Screenshot from 2023-06-21 10-25-13

Step 1

Capture d'écran 2024-02-02 174546

Title: Change editor to CodeMirror
Description: You should change the default editor and select 'CodeMirror'
Target: #jform_editor
Type: interactive
Interactive Type: select list
Options: value required and value to enter: codemirror.

Note: all steps should have the position 'bottom'.

Screenshot from 2023-06-21 10-25-18

Screenshot from 2023-06-21 10-25-22

Step 2

Title: Put the site offline
Description: Now put the site offline
Target: #jform_offline1
Type: interactive
Interactive Type: checkbox/radio
Options: value required.

Screenshot from 2023-06-21 10-25-27

Screenshot from 2023-06-21 10-25-33

Step 3

Title: Change custom message
Description: Please set the custom message to: We will be back soon.
Target: #jform_offline_message
Type: interactive
Interactive Type: text field
Options: value required and value to enter: We will be back soon.

Screenshot from 2023-06-21 10-25-47

Screenshot from 2023-06-21 10-25-53

Step 4

Title: Check the frontend of the site
Description: The tour is now finished - save the configuration and check the site in the frontend

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

Geraint2 and others added 8 commits June 16, 2023 11:47
Add required option for text and radio/checkbox target types to allow us to force the user to interact with the target field even if its not required in the DOM
Reduce duplicated code for enabling/disabling navigation buttons - more can be done!
When target element is specified in a step but it doesn't exist in the DOM we need to end the tour as we have probably navigated to another page
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.0-dev labels Jun 21, 2023
@GeraintEdwards GeraintEdwards marked this pull request as ready for review June 21, 2023 09:41
@GeraintEdwards GeraintEdwards changed the title [5.0] Add checkbox/radio/select target element support, enhanced required field handling [5.0] Guided Tours - Add checkbox/radio/select target element support, enhanced required field handling Jun 21, 2023
@richard67
Copy link
Member

  1. The new "params" column has to be added to the extensions.sql for PostgreSQL, too.
  2. It needs one update SQL script for each kind of database (MySQl and PotsgreSQL) to add the new column to the table.

@richard67
Copy link
Member

  1. Fix PHP code style errors: We use 4 spaces for each level of indentation in PHP files, not tabs.
  2. Fix XML code style.

Add missing params field into postgres sql file
@GeraintEdwards
Copy link
Contributor Author

Thanks @richard67 - I've fixed the missing postgres column, PHP code style and I think I fixed the XML file (the code style guidlines are a bit vague at https://developer.joomla.org/coding-standards/xml.html)

I'm not sure how I would add an update script to add the existing column - I could find no examples in the 5.0dev codebase. Can you point me in the right direction of suggest an example to look at?

@richard67
Copy link
Member

I've fixed the missing postgres column

I can't see that yet.Have you forgotten to push the change?

I'm not sure how I would add an update script to add the existing column - I could find no examples in the 5.0dev codebase. Can you point me in the right direction of suggest an example to look at?

In the 5.0-dev coce base you won't find much because we removed all the 4.x update SQL scripts. But you can find some examples in the 4.4-dev or 4.3-dev code base:

https://github.com/joomla/joomla-cms/blob/4.4-dev/administrator/components/com_admin/sql/updates/mysql/4.3.2-2023-05-03.sql

https://github.com/joomla/joomla-cms/blob/4.4-dev/administrator/components/com_admin/sql/updates/postgresql/4.3.2-2023-05-20.sql

Add missing params field into postgres sql file
@richard67
Copy link
Member

P.S.: The new update SQL script should have a version 5.0.0 and a timestamp newer that the present script "5.0.0-2023-03-11.sql", so I suggest to use "5.0.0-2023-06-21.sql".

Add missing SQL update scripts
@GeraintEdwards
Copy link
Contributor Author

Whoops - postgres change was not pushed :(

Thanks for the update SQL location.

For some reason my PHPStorm installation hadn't picked up on these tabs when I analysed the code :(

@richard67
Copy link
Member

Whoops - postgres change was not pushed :(

Thanks for the update SQL location.

For some reason my PHPStorm installation hadn't picked up on these tabs when I analysed the code :(

There are more code style problems with tabs instead of spaces, but I'm busy at work so can't check further.

If you click the "Details" link at the right side of the "continuous-integration/drone/p" step at the bottom of the PR you come to a page with the CI checks. There you select "PHPCS" at the left hand side, then you see the log of the PHP code style check.

@Quy Quy removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Feb 10, 2024
@obuisard
Copy link
Contributor

Thanks for the reminder Richard @richard67, I totally missed that one :-)

@obuisard Maybe my mistake because I just saw I had not finished that review with that comment so the review was still ongoing and maybe only me could see it.

No, no, my fault, I should know better :-)

@richard67
Copy link
Member

richard67 commented Feb 18, 2024

@GeraintEdwards @obuisard We should not use a NOT NULL constraint for the new column, even if other tables have that for their params column. But e.g. the #__categories table allows NULL values for its params column.

I have made corresponding review comments with change suggestions for that.

The reason is following:

When we add the new column to the table on updates with a NOT NULL constraint, it works on MySQL and MariaDB because that implicitly sets the value to an empty string for that new column.

But on PostgreSQL that does not work. We have to add the column first without the NOT NULL constraint, then update the existing records, and then add the NOT NULL CONSTRAINT. If this fails for some reason, the database fixer will not be able to fix that because it will only run the 2 ALTER TABLE statements but not the UPDATE statement between them, so the 2nd ALTER TABLE will always fail.

Therefore it's the best to allow NULLs, and as far as I can see in the PHP code of this PR it handles that already. But this has to be tested in order to be safe.

@richard67 richard67 added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Feb 18, 2024
obuisard and others added 4 commits February 18, 2024 17:34
…4-02-10.sql

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
…0-2024-02-10.sql

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@richard67 richard67 removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Feb 18, 2024
@obuisard
Copy link
Contributor

@GeraintEdwards @obuisard We should not use a NOT NULL constraint for the new column, even if other tables have that for their params column. But e.g. the #__categories table allows NULL values for its params column.

I have made corresponding review comments with change suggestions for that.

The reason is following:

When we add the new column to the table on updates with a NOT NULL constraint, it works on MySQL and MariaDB because that implicitly sets the value to an empty string for that new column.

But on PostgreSQL that does not work. We have to add the column first without the NOT NULL constraint, then update the existing records, and then add the NOT NULL CONSTRAINT. If this fails for some reason, the database fixer will not be able to fix that because it will only run the 2 ALTER TABLE statements but not the UPDATE statement between them, so the 2nd ALTER TABLE will always fail.

Therefore it's the best to allow NULLs, and as far as I can see in the PHP code of this PR it handles that already. But this has to be tested in order to be safe.

Thank you so much Richard @richard67 for the explanation, it does make perfect sense. Really appreciate your insight on this!

obuisard and others added 2 commits February 18, 2024 18:54
@Quy
Copy link
Contributor

Quy commented Feb 19, 2024

40994-type
Expand the description to include Checkbox/Radio and Select List?

Improved description of the interactive type field
@obuisard
Copy link
Contributor

obuisard commented Feb 19, 2024

40994-type Expand the description to include Checkbox/Radio and Select List?

Good point, thanks!

@LadySolveig LadySolveig merged commit 416bcef into joomla:5.1-dev Feb 19, 2024
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 19, 2024
@LadySolveig
Copy link
Contributor

LadySolveig commented Feb 19, 2024

Thank you so much for this very useful improvements! 🚀 @GeraintEdwards @obuisard and for the nice teamwork and support to @Quy @richard67

@Kostelano
Copy link
Contributor

Kostelano commented Feb 24, 2024

@GeraintEdwards

Good afternoon. I’m not sure yet, I’m still figuring it out, but...

When creating a step, I select the interactive and checkbox/radio, but I don't see the target value in the Target Value Options tab (similar to other interactives).

Screenshot_1

Screenshot_2

Is this expected behavior or a bug?

As far as I understand, from the selection list I can NOT select everything, but only some items.

Kostelano added a commit to JPathRu/localisation that referenced this pull request Feb 24, 2024
@GeraintEdwards
Copy link
Contributor Author

@GeraintEdwards

Good afternoon. I’m not sure yet, I’m still figuring it out, but...

When creating a step, I select the interactive and checkbox/radio, but I don't see the target value in the Target Value Options tab (similar to other interactives).

Is this expected behavior or a bug?

As far as I understand, from the selection list I can NOT select everything, but only some items.

At present the code allows one selector - the specific checkbox/radiobox and the required option needs this to be checked. I guess it may make sense to generalise this so allow for the requirement that the checkbox/radiobox to be unchecked and also to allow require multiple options to be checked (or selected in select list fields). This is more complex to implement as the values are not available to us when editing the steps.

It would, of course, be really neat if we could create/edit the tour and its steps in-situ so that we could pick up the values from the form in real time using javascript. But I think that is a step to advanced for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Language Change This is for Translators 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.