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] Remove obsolete menu class "nav-pills" from menu modules' parameters and contact links #23055

Merged
merged 4 commits into from Dec 14, 2018

Conversation

@richard67
Copy link
Member

richard67 commented Nov 12, 2018

Pull Request for Issue #23023 .

Summary of Changes

Remove the obsolete menu class "nav-pills" from contact default links markup and blog sampledata plugin menu creation.

Testing Instructions

Code review for the sample data part, patch tester and check that contact links look like without this PR for the contact links part.

Expected result

The menu class "nav-pills" is not added in blog sample data for menus at module position "top-a", e.g. the "Main Menu Blog", and also not added to the markup of the contact links list (<ul> element).

Actual result

The menu class "nav-pills" is added in blog sample data for menus at module position "top-a", e.g. the "Main Menu Blog", but it has no effect.
This might confuse administrators who create or maintain menu modules.
The CSS class nav-pills" is also added to the markup of the contact links list (<ul> element), but it has no effect there because the links list doesn't have an active element on which the pills effect should be applied.

Documentation Changes Required

None.

Remove menu class nav-pills from contact default links markup and blog sampledata plugin menu creation.
@richard67

This comment has been minimized.

Copy link
Member Author

richard67 commented Nov 12, 2018

@ciar4n Could you review or test this PR?


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

@@ -25,7 +25,7 @@
<?php endif; ?>

<div class="com-contact__links contact-links">
<ul class="nav nav-pills flex-column">

This comment has been minimized.

Copy link
@ciar4n

ciar4n Nov 12, 2018

Member

The reason for nav-pills not working from the database should not apply here.

This comment has been minimized.

Copy link
@richard67

richard67 Nov 12, 2018

Author Member

Well, but this class has no effect in the Cassiopeia template. Compare the look of contact links with and without nav-pills, and you will not see any difference. That's why I think it should be removed here, too.

This comment has been minimized.

Copy link
@ciar4n

ciar4n Nov 12, 2018

Member

I see now. Bootstraps nav-pills class applies the pill effect to 'active' items only. There is no 'active' items in this list so you are correct.

@richard67 richard67 changed the title [4.0] Remove obsolete menu class "nav-pills" from menu modules' parameters [4.0] Remove obsolete menu class "nav-pills" from menu modules' parameters and contact links Nov 12, 2018
@ciar4n

This comment has been minimized.

Copy link
Member

ciar4n commented Nov 12, 2018

I have tested this item successfully on 152e183


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

@richard67

This comment has been minimized.

Copy link
Member Author

richard67 commented Nov 17, 2018

@ciar4n Could you set your test result again? It has been reset by branch update to latest 4.0-dev. There have not been made any changes. Thanks in advance.


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

@ciar4n

This comment has been minimized.

Copy link
Member

ciar4n commented Nov 17, 2018

I have tested this item successfully on 9c2f072


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

@richard67

This comment has been minimized.

Copy link
Member Author

richard67 commented Nov 17, 2018

Thanks a lot. Hope we find a 2nd tester.

@richard67

This comment has been minimized.

Copy link
Member Author

richard67 commented Nov 24, 2018

@ciar4n Sorry, could you set your test result again? I had again to update branch. Thanks in advance.
@Quy Do you have time and mood to test this PR?

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Nov 24, 2018

I have tested this item successfully on 5dcd2da


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

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Nov 24, 2018

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC label Nov 24, 2018
@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Nov 24, 2018

Updated @ciar4n test result in Issues.
Relaunched Travis to try to make it pass as the analysis4x issue is not related.

@richard67

This comment has been minimized.

Copy link
Member Author

richard67 commented Nov 24, 2018

@infograf768 Thanks a lot.

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Nov 24, 2018

@richard67
Maybe you have time to test:
#23143 (RTL stuff too)

@wilsonge wilsonge merged commit c32bb41 into joomla:4.0-dev Dec 14, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Dec 14, 2018

Thanks guys! Sorry for the delay on merging

@joomla-cms-bot joomla-cms-bot removed the RTC label Dec 14, 2018
@wilsonge wilsonge added this to the Joomla 4.0 milestone Dec 14, 2018
@richard67 richard67 deleted the richard67:remove-menu-class-nav-pills branch Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.