Skip to content

Comments

[4.0] wrap message container output in an array and implode it so it prevents obsolete spaces#30760

Merged
richard67 merged 12 commits intojoomla:4.0-devfrom
hans2103:trim-system-message
Oct 21, 2020
Merged

[4.0] wrap message container output in an array and implode it so it prevents obsolete spaces#30760
richard67 merged 12 commits intojoomla:4.0-devfrom
hans2103:trim-system-message

Conversation

@hans2103
Copy link
Contributor

Pull Request for Issue #30759 .

Summary of Changes

When styling the messages and spacing around it I would like to be able to hide the message box when empty using the following CSS. It is clean, it is simple...

#system-message:empty {
    display: none;
}

Because the opening of PHP starts on a new line there is some obsolete spacing rendered.
The CSS statement above is not working due to these obsolete space characters.
When PR is merged the empty message box will appear together with its spacing.
This PR moves the rendering of the joomla-alert to its own JLayout.
The idea behind this is that <div id="system-message"><?php echo $output; ?></div> can be a oneliner which collapses to the AFTER result when rendered empty.

Testing Instructions

  • Joomla 4 install
  • Inspect element

Actual result BEFORE applying this Pull Request

<div id="system-message">
			</div>

Expected result AFTER applying this Pull Request

<div id="system-message"></div>

Documentation Changes Required

@HLeithner
Copy link
Member

I don't see much benefit from this except if you want to style your system messages different you have to override multiple files

@zero-24
Copy link
Contributor

zero-24 commented Sep 25, 2020

Do you? You can still just override the first and dont load the seccond one in that override right?

@hans2103
Copy link
Contributor Author

The benefit is having <div id="system-message"></div> in your html instead of <div id="system-message"> </div>
The difference is the appearance of obsolete spacing.
And due to this obsolete spacing a simply hide using #system-message:empty {display: none;} cannot be done. The obsolete spacing makes the element not empty.

@HLeithner
Copy link
Member

Just fix fix the markup in the first file. But creating an additional file for only copy the content to the new file doesn't make much sense to me.

@hans2103
Copy link
Contributor Author

@HLeithner sorry... I had no intention to add the extra tabs at the beginning of the file. They have been removed with commit 1ce10bb

But the onliner <div id="system-message"><?php echo $output; ?></div> is needed to get <div id="system-message"></div> while respecting Joomla Code Style

@hans2103
Copy link
Contributor Author

Thank you @HLeithner for the suggestion of this easy solution. 👍

@brianteeman
Copy link
Contributor

Could you update the title please

@SharkyKZ
Copy link
Contributor

JS needs updating.

Though I don't think is a good idea at all. Relying on markup to contain/not contain whitespaces is too volatile. Wouldn't use :empty until it supports whitespaces.

@hans2103 hans2103 changed the title move joomla-alert from message to it's own JLayout add else statement to render message container without obsolete spaces Sep 25, 2020
@hans2103
Copy link
Contributor Author

:empty that matches whitespace is not in any browser at the moment.
https://caniuse.com/mdn-css_selectors_empty_matches_whitespace

Though this simple else statement can help frontenders to get rid of obsolete margins.

.container-component {
  > * {
    margin-bottom: 0;
  }
  
  > * + * {
    margin-top: 1.5rem;
  }
}

#system-message:empty {
  display: none;
}

please consider merging this PR. It will benefit styling

@hans2103 hans2103 changed the title add else statement to render message container without obsolete spaces [4.0] Add else-statement to render message container without obsolete spaces Sep 25, 2020
@SharkyKZ
Copy link
Contributor

:empty that matches whitespace is not in any browser at the moment.
https://caniuse.com/mdn-css_selectors_empty_matches_whitespace

Yes, I know.

@hans2103
Copy link
Contributor Author

hans2103 commented Sep 25, 2020

The implementation of :empty allowing whitespace is not even close. In the meanwhile we can make Joomla for everyone and merge this PR. A lot of frontend developers would benefit.

JS needs updating.

@SharkyKZ can you explain this? Where and why does JS need to be updated?

@HLeithner
Copy link
Member

Btw, we have code in mod_menu (maybe not anymore) with a comment that the markup shouldn't have any whit-spaces.

If needed ok but if not really necessary I'm not sure we should rely on such restriction.

@HLeithner
Copy link
Member

maybe it would better to set a class if empty and a class if content is added with js?

@hans2103
Copy link
Contributor Author

hans2103 commented Sep 25, 2020

@HLeithner fine by me... If I just can remove spacing when there are no alerts to be shown.
But changing js depends on js, while this PR only relies on PHP

please make a PR on mine with your suggested change

@SharkyKZ
Copy link
Contributor

The implementation of :empty allowing whitespace is not even close. In the meanwhile we can make Joomla for everyone and merge this PR. A lot of frontend developers would benefit.

JS needs updating.

@SharkyKZ can you explain this? Where and why does JS need to be updated?

When you close the messages, the div contains whitespaces.

@hans2103
Copy link
Contributor Author

@SharkyKZ So the suggestion of @HLeithner to toggle a className when messages are shown or not is an option?
I don't have enough JS skills to do so, if you and / or @HLeithner can help me with it. Would be helpful.

@ceford
Copy link
Contributor

ceford commented Sep 26, 2020

I have tested this item ✅ successfully on e440e42

If the system message container is empty why is it rendered at all?

  <div id="system-message-container" aria-live="polite">
  	<div id="system-message"></div>
  </div>

I don't see any visible differences in front or back-end (in Firefox/Mac).


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

@hans2103
Copy link
Contributor Author

hans2103 commented Oct 18, 2020

so if you clean the PHP code you will still have a whitespace explained by skarky (didn't tested it my self)

I don't think so.
The error output generated by my PR is as follows

<div id="system-message-container" aria-live="polite"><div id="system-message"><joomla-alert type="error" dismiss="true"><div class="alert-heading"><span class="error"></span><span class="sr-only">error</span></div><div class="alert-wrapper"><div class="alert-message">**some kind of error message**</div></div></joomla-alert></div></div>

When the message is removed by clicking the x the entire block will collapse to:

<div id="system-message-container" aria-live="polite"></div>

(I have tested this)

As you can see there is no obsolete space within this element. And that is what I want to achieve. No obsolete space.
And this can be achieved without adding javascript

@Quy
Copy link
Contributor

Quy commented Oct 19, 2020

To reproduce, edit/save an article.

30760

@hans2103
Copy link
Contributor Author

@Quy
solved the issue with commit 88f1568 by changing

$msgOutput[] = '<joomla-alert type="' . $alert[$type] ?? $type . '" dismiss="true">';

into

$msgOutput[] = '<joomla-alert type="' . ($alert[$type] ?? $type) . '" dismiss="true">';

@Quy Quy removed the RTC This Pull Request is Ready To Commit label Oct 21, 2020
@richard67
Copy link
Member

@hans2103
Copy link
Contributor Author

I see the message... but there are no JS changes in this PR.

@richard67
Copy link
Member

@hans2103 Maybe it helps if you update the branch to latest 4.0-dev?

@hans2103
Copy link
Contributor Author

merged with latest changes from 4.0-dev

@richard67
Copy link
Member

merged with latest changes from 4.0-dev

Seems it helped 😄

@Quy
Copy link
Contributor

Quy commented Oct 21, 2020

I have tested this item ✅ successfully on c498183


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

1 similar comment
@drmenzelit
Copy link
Contributor

I have tested this item ✅ successfully on c498183


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

@Quy
Copy link
Contributor

Quy commented Oct 21, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 21, 2020
@richard67 richard67 merged commit a4de85a into joomla:4.0-dev Oct 21, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 21, 2020
@richard67
Copy link
Member

Thanks!

@richard67 richard67 added this to the Joomla 4.0 milestone Oct 21, 2020
@SharkyKZ
Copy link
Contributor

🤦‍♂️

@richard67
Copy link
Member

@SharkyKZ It's bad? Have I missed some open discussion? If so, could you make a new issue for it?

@richard67
Copy link
Member

Hmm, I see, it was hidden by default because too long comment history.

@HLeithner Shall we roll back?

@hans2103 hans2103 deleted the trim-system-message branch October 21, 2020 21:17
@hans2103
Copy link
Contributor Author

@SharkyKZ wrote: #30760 (comment)

system-message selector isn't used in JS or in our new templates (only in system ) so could probably be removed.

In Joomla 3 the file media/plg_installer_webinstaller/js/client.js line 81 makes a reference to #system-message.
And there are some css references to this id.

In Joomla 4 there is no Javascript reference to #system-message anymore.
Might this div element with id = system-message be obsolete?
I'll create a new PR to perform some house cleaning.

@hans2103
Copy link
Contributor Author

PR #31195 created to remove obsolete div element with id = system-message and remove obsolete css referring to this element.

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.