Skip to content
This repository was archived by the owner on Jan 25, 2021. It is now read-only.

Comments

moving layout styling from grid to layout. Fix spacing above message box#132

Merged
richard67 merged 4 commits intojoomla:developmentfrom
hans2103:feature-cassiopeia/#115--Padding-Top-system-message-container
Sep 30, 2020
Merged

moving layout styling from grid to layout. Fix spacing above message box#132
richard67 merged 4 commits intojoomla:developmentfrom
hans2103:feature-cassiopeia/#115--Padding-Top-system-message-container

Conversation

@hans2103
Copy link
Collaborator

Pull Request for Issue #115 .

Summary of Changes

Testing Instructions

Repeat changes stated in #115

Expected result

Schermafbeelding 2020-09-25 om 08 32 36

Actual result

as shown in #115

ALERT

Please have a look at joomla/joomla-cms#30760
It will solve the obsolete spacing above the alert when no alert has to be shown.

Schermafbeelding 2020-09-25 om 08 27 16

@richard67
Copy link
Member

This PR also changes file administrator/templates/atum/scss/vendor/joomla-custom-elements/joomla-alert.scss. Has anyone tested if that has any unwanted effects in the backend?

@richard67
Copy link
Member

And shall we really change atum files in this project?

@richard67
Copy link
Member

Up to now we don't modify and files belonging to the backend template with our works, and I don't wanna start with it now.

If we change backend template stuff with a CMS PR for the frontend template it will be hardly accepted.

@hans2103 Can we make things work for Cassiopeia without the change in file administrator/templates/atum/scss/vendor/joomla-custom-elements/joomla-alert.scss? If so, please do so. Thanks in advance.

I agree not to touch Atum in this repo
@hans2103
Copy link
Collaborator Author

@richard67 I agree and reverted the Atum change

@richard67
Copy link
Member

@drmenzelit Can you do also a real, visual test (review approval means for me only code review)? If so, report back if it was ok or not. Thanks in advance.

@drmenzelit
Copy link
Collaborator

I have now too much space above the breadcrumbs
grafik

@richard67
Copy link
Member

I have now too much space above the breadcrumbs

@drmenzelit I think this is related to following item in the description of this PR:

Unfortunately I'm not sure if that PR joomla/joomla-cms#30760 will make it in the CMS. There is an ongoing discussion in it.

@hans2103
Copy link
Collaborator Author

hans2103 commented Sep 30, 2020

I cannot reproduce issue with too many spacing above breadcrumbs.
Not anymore... in branch development the messages have been moved below the breadcrumbs

I do see too much spacing between breadcrumbs and output of component.
That is due to the the obsolete spacing in an empty system-message. Trying to solve it in joomla/joomla-cms#30760
Schermafbeelding 2020-09-30 om 16 37 32

@drmenzelit
Copy link
Collaborator

Ok, in my installation the message was above the bredcrumbs. Then is the PR ok.

@richard67
Copy link
Member

I have tested this PR ✅ with success.

Since there were changes meanwhile, the breadcrumbs comes before the message container now, so this PR solves the issue with missing spacing above the bradcrumbs and not as described with the message container.

@richard67 richard67 merged commit 1436dff into joomla:development Sep 30, 2020
@richard67
Copy link
Member

Thanks!

@hans2103 hans2103 deleted the feature-cassiopeia/#115--Padding-Top-system-message-container branch September 30, 2020 22:02
@Scrabble96
Copy link
Contributor

Scrabble96 commented Oct 21, 2020

On the Cassiopeia PR #48 installation as of 20th Oct (I am assuming that this build included the above merge) there is no longer any padding above the system message. However, there is a margin top of 1em above the system-message-container. Perhaps this should be moved to the system-message div inside, so when/if they sort out the do-not-display-when-empty issue, there will be no surplus margin.

system-message-container

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants