Skip to content

Conversation

@richard67
Copy link
Member

@richard67 richard67 commented Jul 4, 2019

Pull Request for Issue #25415 .

Summary of Changes

  1. Change icon and class for the message related to contact errors in the multilanguage status module.
    2. Remove text "Warning! " from the beginning of the language string for that message.

These 2 changes are made with separate commits for each change, so if it turns out that only one of them is accepted, then it can be cherry-picked.

Testing Instructions

See issue #25415 .

Expected result

Consistent display of all warnings, i.e. show contact related warning like all the others with warning class and icon but no text "Warning! " at the beginning.

Actual result

All warnings have warning class and icon and no text "Warning! " at the beginning. Only the text related to contact errors has class and icon for info but text "Warning! " at the beginning.

Documentation Changes Required

None.

@richard67
Copy link
Member Author

richard67 commented Jul 4, 2019

@brianteeman I'd like to know your opinion if both changes, 1. icon and class, and 2. the removal of "Warning! " from the beginning of the text are good, or if you think only one of them is better. I think both together is most consistent with other warnings in the multilanguage status module's modal. I've made separate commits so each change can be cherry-picked.

@brianteeman
Copy link
Contributor

In joomla 3 you will have to keep the word"warning" in the text. In joomla 4 these messages should all have a headline saying "warning" (and if they dont yet then its just because its on my todo)

The other changes are fine and enough to resolve this issue

@richard67
Copy link
Member Author

richard67 commented Jul 4, 2019

@brianteeman Hmm, the other texts dont's start with "Warning! ". What these warnings all have is a hidden (class="element-invisible") span with JText::_('WARNING'), and so I've changed that hidden span for the questionable warning from JText::_('INFO') to JText::_('WARNING'), Is it that what you mean?

@brianteeman
Copy link
Contributor

and the others are wrong which is why I fixed them in j4 ;)

@richard67
Copy link
Member Author

Ahh. Got it. Will undo the corresponding commit to remove the test part.

@brianteeman
Copy link
Contributor

The hidden span is for screen reader users as they cant see the icon or colour

@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Jul 4, 2019
@richard67
Copy link
Member Author

Yes, that's why I changed that, too. Thanks for advise. I really appreciate that. PR should be ready now.

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 19ba859


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

1 similar comment
@alikon
Copy link
Contributor

alikon commented Jul 4, 2019

I have tested this item ✅ successfully on 19ba859


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

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Jul 4, 2019
@alikon
Copy link
Contributor

alikon commented Jul 4, 2019

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 4, 2019
@ghost
Copy link

ghost commented Jul 5, 2019

Thanks Guys for your Work.

@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Jul 5, 2019
@HLeithner
Copy link
Member

thx

@HLeithner HLeithner merged commit 7d084b1 into joomla:staging Jul 20, 2019
@HLeithner HLeithner added this to the Joomla! 3.9.11 milestone Jul 20, 2019
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 20, 2019
@richard67 richard67 deleted the staging-correct-multilang-contact-warning branch July 20, 2019 18:59
@richard67
Copy link
Member Author

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants