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

[5.2] [Guided tours] Tour header visual changes + footer padding fix #43809

Merged
merged 15 commits into from
Aug 9, 2024

Conversation

obuisard
Copy link
Contributor

@obuisard obuisard commented Jul 18, 2024

Summary of Changes

The guided tours header does not have a clear indication that a tour can be cancelled. There is no label and the progress indicator is next to it, making it hard to distinguish.
The changes include:

  • A Cancel label close to the cancel cross to provide context and enhance usability,
  • moving the progress indicator away from the cancel button to ensure the cancel is clearly visible.

Testing Instructions

It is just a visual change. Make sure you can still cancel a tour, no matter if you are selecting the cross or the label.

Actual result BEFORE applying this Pull Request

image

Expected result AFTER applying this Pull Request

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
@obuisard obuisard changed the title [Guided tours] Tour header visual changes [5.2] [Guided tours] Tour header visual changes Jul 18, 2024
Changed the order of the properties.
@obuisard obuisard marked this pull request as ready for review July 20, 2024 02:45
@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on a19918e


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

@brianteeman
Copy link
Contributor

You need to use logical css properties otherwise your css breaks on RTL

image

example
change margin-right to margin-inline-end

@obuisard
Copy link
Contributor Author

obuisard commented Jul 20, 2024

You need to use logical css properties otherwise your css breaks on RTL
example change margin-right to margin-inline-end

I have adjusted the CSS. Thanks!

image

@brianteeman
Copy link
Contributor

Thanks for the last change.

I am not a designer but the header looks odd to me as we have multiple text sizes and line heights on the same line which results in 4 different baselines

You can see it when you enlarge the header and draw some lines

image

@obuisard
Copy link
Contributor Author

Thanks for the last change.

Of course! I am glad you pointed it out.

I am not a designer but the header looks odd to me as we have multiple text sizes and line heights on the same line which results in 4 different baselines
You can see it when you enlarge the header and draw some lines

That's because the elements in the header are vertically aligned in the center of the top container.
I was not 'happy' with the 'Cancel' label, however. I changed it a bit.
Looks better to my eye.

image

@Quy
Copy link
Contributor

Quy commented Jul 20, 2024

Please consider hiding Cancel as this is not shown in other modals.

@obuisard
Copy link
Contributor Author

obuisard commented Jul 21, 2024

Please consider hiding Cancel as this is not shown in other modals.

It was hidden before, one of the ideas for this PR is to make it visible. These are guided tours and from the start some of the user feedback was that the X is not a clear indication that it is meant to cancel tours (or close them). It helps teaching newbies that yes, the 'X' cancels/closes the window.

@glarkell
Copy link

Tested successfully on J5.2.

@Kostelano
Copy link
Contributor

I haven’t tested it yet, but what if the step title is long enough?

@Kostelano
Copy link
Contributor

I haven’t tested it yet, but what if the step title is long enough?

I can say that this is a rather short title in Russian. Now it turns out that almost every step has a line break, which doesn’t look very good to me. Most likely there will be lines with 3/4 line breaks.

I would prefer that somehow this could be solved.

Perhaps enlarge the window. It is still possible to remove CANCEL, since all other modal windows simply have a cross. It is possible to move CANCEL down to the button block (from START).

Screenshot_2

@sdwjoomla
Copy link
Contributor

Please consider hiding Cancel as this is not shown in other modals.

Having the 'Cancel' next to the 'X' helps answer the question received for clients who noticed it and aske what it does. For the client who hadn't noticed the 'X' and would also ask how to cancel a tour, adding the word 'Cancel' makes it easier for them.

@sdwjoomla
Copy link
Contributor

@Kostelano Thanks for taking a look. Yes, the line break doesn't look the best. There is a companion PR to this one, #43810 which addresses the width. Would you have a few moments to take a look at it?

@Quy
Copy link
Contributor

Quy commented Jul 21, 2024

The Step badge now takes up more space and has the main focus when it should be the title. Maybe move it to the bottom left corner.

The Cancel text is competing for attention with the title. You want them to do the tour but constantly see the Cancel with each step. It needs to be more subtle as before.

@brianteeman
Copy link
Contributor

Accessibility says that you should always indicate how far along in a process you are so leading with the step count is good. I agree that it is not needed to display the cancel text. We don't do it on other modals

@chmst
Copy link
Contributor

chmst commented Jul 21, 2024

I agree that it is not needed to display the cancel text. We don't do it on other modals

agreed

@Quy
Copy link
Contributor

Quy commented Jul 21, 2024

Not aligned.

43809-start

43809-end

@obuisard
Copy link
Contributor Author

obuisard commented Jul 22, 2024

Not aligned.

Thanks for the catch!

I made a correction in PR #43810, which is were the padding was changed for the text content.
I have adjusted the footer padding in this PR, which was not aligned since 4.3 :-)

@obuisard obuisard changed the title [5.2] [Guided tours] Tour header visual changes [5.2] [Guided tours] Tour header visual changes + footer padding fix Jul 22, 2024
obuisard added a commit to obuisard/joomla-cms that referenced this pull request Jul 22, 2024
@Quy
Copy link
Contributor

Quy commented Jul 23, 2024

Perform How to add steps to a guided tour?. Missing X and partial X.

43809-step1

43809-step2

@obuisard
Copy link
Contributor Author

obuisard commented Jul 23, 2024

Perform How to add steps to a guided tour?. Missing X and partial X.

Strange, I can't reproduce it. No matter the window size. What browser are you using? Anybody else with the issue?

image

image

The header is a flex container so it's children should resize appropriately...

@Quy
Copy link
Contributor

Quy commented Jul 23, 2024

Firefox 128.0.2 and Chrome Version 127.0.6533.73 on Windows

43809-css

@Quy
Copy link
Contributor

Quy commented Jul 23, 2024

The last commit is causing the issue.

@obuisard
Copy link
Contributor Author

The last commit is causing the issue.

Thanks, I corrected it.

@Quy
Copy link
Contributor

Quy commented Jul 27, 2024

I have tested this item ✅ successfully on 0562ade


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

1 similar comment
@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on 0562ade


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

@Quy
Copy link
Contributor

Quy commented Jul 28, 2024

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 28, 2024
pe7er pushed a commit that referenced this pull request Aug 9, 2024
…popup (#43810)

* Add sizes to the shepherd element

* Changed the order of the properties.

* Update guidedtours.scss

* Added fix for the popup height in case the content gets taller than the viewport

* Changed property order

* Correct padding for text

* Corrections padding footer

* Use shorthand

* Added footer padding again to accommodate this PR, although added to PR #43809

* Improve the layout of the intro.

* Update guidedtours.joomla_welcome_steps.ini

* Improve the layout of the dashboard panel step

* use <br> rather than <br />
@pe7er pe7er enabled auto-merge (squash) August 9, 2024 11:36
@pe7er pe7er disabled auto-merge August 9, 2024 11:40
@pe7er pe7er self-assigned this Aug 9, 2024
@pe7er pe7er enabled auto-merge (squash) August 9, 2024 12:17
@pe7er pe7er merged commit d301e0f into joomla:5.2-dev Aug 9, 2024
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 9, 2024
@Quy Quy added this to the Joomla! 5.2.0 milestone Aug 9, 2024
@pe7er
Copy link
Contributor

pe7er commented Aug 9, 2024

Thanks @obuisard !

@obuisard obuisard deleted the guided-tours-header 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
NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.2-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.