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

[4.0] Make alert inline consistent #27707

Merged
merged 5 commits into from
Feb 12, 2020
Merged

[4.0] Make alert inline consistent #27707

merged 5 commits into from
Feb 12, 2020

Conversation

Quy
Copy link
Contributor

@Quy Quy commented Jan 29, 2020

Summary of Changes

Make alert inline consistent like this.

no-matching-results

Testing Instructions

Go to System > Warnings

Expected result

alert-after

Actual result

alert-before

@brianteeman
Copy link
Contributor

Goof spot but you need to fix the other ones in the same file as well

@Quy
Copy link
Contributor Author

Quy commented Jan 29, 2020

I am not sure how to handle these.

The first one has dynamic heading and description. I will have to see what possible headings are.

For the second one, can the heading be replaced with the i icon?

COM_INSTALLER_MSG_WARNINGFURTHERINFO="Further information on warnings"

						<?php foreach ($this->messages as $message) : ?>
							<div class="alert alert-warning">
								<h4 class="alert-heading"><?php echo $message['message']; ?></h4>
								<p class="mb-0"><?php echo $message['description']; ?></p>
							</div>
						<?php endforeach; ?>
						<div class="alert alert-info">
							<h4 class="alert-heading"><?php echo Text::_('COM_INSTALLER_MSG_WARNINGFURTHERINFO'); ?></h4>
							<p class="mb-0"><?php echo Text::_('COM_INSTALLER_MSG_WARNINGFURTHERINFODESC'); ?></p>
						</div>

@brianteeman
Copy link
Contributor

just about to start a presentation. i wont be able to take a look now until tomorrow

@brianteeman
Copy link
Contributor

For the first one I think we need to keep the heading but we could include the relevant icon for some consistency

image

For the second message you are correct

Note the exact same code is used in com_joomlaupdate so that would need updating as well

@chmst
Copy link
Contributor

chmst commented Feb 12, 2020

I have tested this item ✅ successfully on 5aecd4c


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

1 similar comment
@jwaisner
Copy link
Member

I have tested this item ✅ successfully on 5aecd4c


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

@jwaisner
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 12, 2020
@brianteeman
Copy link
Contributor

The exact same code in joomlaupdate should be changed as well

@rdeutz rdeutz added this to the Joomla 4.0 milestone Feb 12, 2020
@rdeutz rdeutz merged commit 8b46e88 into joomla:4.0-dev Feb 12, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 12, 2020
@Quy Quy deleted the alert-inline branch February 12, 2020 20:58
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.

6 participants