Skip to content

Fix of vertical line not having height#33821

Merged
rdeutz merged 2 commits intojoomla:4.0-devfrom
eopws:autum-joomla-tab-layout-minor-change
May 14, 2021
Merged

Fix of vertical line not having height#33821
rdeutz merged 2 commits intojoomla:4.0-devfrom
eopws:autum-joomla-tab-layout-minor-change

Conversation

@eopws
Copy link
Copy Markdown

@eopws eopws commented May 12, 2021

Pull Request for Issue #33809.

Summary of Changes

Vertical line that separates sidebar from content is extended to bottom (see screenshots and issue)

Testing Instructions

Go to Global Configuration > any component

Actual result BEFORE applying this Pull Request

The vertical line lack of height
image

Expected result AFTER applying this Pull Request

Everything is fine with height
after

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels May 12, 2021
@eopws eopws changed the title Added height property Fix of vertical line height not having height May 12, 2021
@eopws eopws changed the title Fix of vertical line height not having height Fix of vertical line not having height May 12, 2021
@brianteeman
Copy link
Copy Markdown
Contributor

Wasnt this by design?

@richard67
Copy link
Copy Markdown
Member

@eopws Code style error reported by drone in the scss-cs step, i.e. scss code style:

administrator/templates/atum/scss/vendor/joomla-custom-elements/joomla-tab.scss
--
 304:5  ✖  Expected "height" to come before "border-left"   order/properties-order

Made this because of linter asks to do it
@eopws
Copy link
Copy Markdown
Author

eopws commented May 12, 2021

Wasnt this by design?

I don't think so, because the line was created to separate sidebar from content and logically should be extended to the bottom

@brianteeman
Copy link
Copy Markdown
Contributor

perhaps, perhaps not

@ghost
Copy link
Copy Markdown

ghost commented May 12, 2021

My first impression of the issue was like @eopws but i would like to get a comment by the designer @ciar4n.

@ceford
Copy link
Copy Markdown
Contributor

ceford commented May 13, 2021

I have tested this item ✅ successfully on 7468f97

It works. So OK if accepted by designers.


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

@Quy
Copy link
Copy Markdown
Contributor

Quy commented May 13, 2021

I have tested this item ✅ successfully on 7468f97


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

@richard67
Copy link
Copy Markdown
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 13, 2021
@richard67 richard67 added this to the Joomla 4.0 milestone May 13, 2021
@rdeutz rdeutz merged commit 9bbca25 into joomla:4.0-dev May 14, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 14, 2021
@brianteeman
Copy link
Copy Markdown
Contributor

So much for waiting for feedback to see if it was by design. Just because something gets two successful tests does not mean it should be merged.

@ciar4n
Copy link
Copy Markdown
Contributor

ciar4n commented May 14, 2021

From a design perspective I would say this was indeed an issue to be fixed 👍

@brianteeman
Copy link
Copy Markdown
Contributor

Thank you for confirming

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants