Skip to content

Conversation

@obuisard
Copy link
Contributor

@obuisard obuisard commented Jul 18, 2024

Summary of Changes

When creating tours, media content is very limited with the size of the popup.
Shepherd forces a popup width to be 400px max.
This PR proposes to increase the width to 600px and a popup that resizes according to its content.
In the case of a popup positioned a the center of the window, the max width could be increased even more (up to 1200px).

Testing Instructions

Run a few tours and see if the popups resize properly with different window sizes.
Add an image to a tour, run the tour and look at the result, either in a central position or at a bottom position.

Actual result BEFORE applying this Pull Request

image

image

image

Expected result AFTER applying this Pull Request

The content dictates the width of the popups, up to 600px, except for the center popups, up to 1200px.

image

image

image

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

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.2-dev labels Jul 18, 2024
@brianteeman
Copy link
Contributor

How do you add an image. Every time I insert an image into the description and then save it is removed

@obuisard
Copy link
Contributor Author

obuisard commented Jul 19, 2024

How do you add an image. Every time I insert an image into the description and then save it is removed

Hi Brian, I use
image

I have not seen the behavior you are experiencing so far.
I think the removal of the image tag could be an editor setting issue.

@brianteeman
Copy link
Contributor

tried on two different installs on different servers and the image is removed on save
Even tried without any editor and still no luck

tour

@obuisard
Copy link
Contributor Author

obuisard commented Jul 19, 2024

tried on two different installs on different servers and the image is removed on save Even tried without any editor and still no luck

Thanks for the gif.
I believe that is because you left the language key in the content. Have you tried without it? The language key takes precedence over the rest.

@brianteeman
Copy link
Contributor

yes that was it. strange that you can add text to the language key but not an image

@obuisard
Copy link
Contributor Author

yes that was it. strange that you can add text to the language key but not an image

Yes, it has to be improved...

@obuisard obuisard marked this pull request as ready for review July 20, 2024 02:46
@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 3a99243


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

@brianteeman
Copy link
Contributor

It works fine when using a landscape image as shown in your instructions. However if you have a large portrait image it causes problems. Not least of which is that you can not exit the tour as you cannot reach the buttons.

Before PR

image

After PR

image

obuisard added 2 commits July 20, 2024 18:28
Added fix for the popup height in case the content gets taller than the viewport
@obuisard
Copy link
Contributor Author

obuisard commented Jul 20, 2024

It works fine when using a landscape image as shown in your instructions. However if you have a large portrait image it causes problems. Not least of which is that you can not exit the tour as you cannot reach the buttons.

Yes, we had the problem before, even before this PR. The PR increases the width, so it makes the behavior even worse.
So I have added a fix for the height, so every tall popup shows more 'elegantly'.

image

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on ed67f8d


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

@obuisard obuisard changed the title [5.2] [Guided tours] Increase the width of the popup [5.2] [Guided tours] Increase the width and adjust the height of the popup Jul 21, 2024
@Kostelano
Copy link
Contributor

It is not entirely clear what the width of the window depends on. I just used the basic greeting tour and the blocks with roughly the same amount of text were very different in width. In addition, the buttons also “stretched out”.

There is no indentation below from the main block to the buttons. Previously he was.

Screenshot_2

Screenshot_3

@obuisard
Copy link
Contributor Author

obuisard commented Jul 22, 2024

It is not entirely clear what the width of the window depends on. I just used the basic greeting tour and the blocks with roughly the same amount of text were very different in width. In addition, the buttons also “stretched out”.

The popup resizes according to its content with this PR.
When targeting an element on the page, the popup resizes and is not allowed to be bigger than 600px (going over 600px is problematic when the popup needs to show on the left or the right of an element, it can go out of space really fast).
When NOT targeting any element on the page (the popup is showing in the center of the page), the popup can resize up to 1200px (depending also on how large the viewport is).

This is meant to allow more verbose content, images and videos ('what's new' tours -coming soon - will be more verbose).

People creating tours can visually tweak the tours to show 'elegantly' with this new behavior. It really depends on how the step content is formatted.
All core tours show 'nicely' out of the box with this new change, except for the welcome tour which could use a couple <br /> tags in the content of the 1st and 3rd steps to avoid a long line and expand to full width.

As far as button stretch, we left the default behavior where buttons adapt to the popup width.

@obuisard
Copy link
Contributor Author

There is no indentation below from the main block to the buttons. Previously he was.

Screenshot_2

I put the padding back, even though it was fixed in PR #43809. Padding needs to be set in both PRs so that content shows as intended when PRs are run separately.

Improve the layout of the intro.
@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Jul 22, 2024
@webiedesign
Copy link

I have tested this item ✅ successfully on 479d165

I added images to 2 guided tours and they adapted to my screen resolution up to aprox 1200. Mobile display resized appropriately and the targets remained where they should have.


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

@AboutTimeIT
Copy link

I have tested this item ✅ successfully on 4906557

Tested it in ‎5.2.0-alpha3 with a 1280x683 image and a 1070x4182.
The landscape image is resized within the instructionswindow. The portrait image eil get scrollbars on the side of the main window so that the buttons stay available.
The Guided Tours windows stay on their positions when the images are inserted.


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

@Quy
Copy link
Contributor

Quy commented Aug 8, 2024

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 8, 2024
@pe7er pe7er enabled auto-merge (squash) August 9, 2024 09:05
@pe7er pe7er merged commit 45ea98b into joomla:5.2-dev Aug 9, 2024
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 9, 2024
@pe7er
Copy link
Contributor

pe7er commented Aug 9, 2024

Thanks @obuisard !

@Quy Quy added this to the Joomla! 5.2.0 milestone Aug 9, 2024
@obuisard obuisard deleted the guided-tours-popup-size branch October 29, 2024 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

8 participants