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 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
Copy link
Member Author

@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">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Member Author

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

@richard67
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member

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 This Pull Request is Ready To Commit label Nov 24, 2018
@infograf768
Copy link
Member

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

@richard67
Copy link
Member Author

@infograf768 Thanks a lot.

@infograf768
Copy link
Member

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

@wilsonge wilsonge merged commit c32bb41 into joomla:4.0-dev Dec 14, 2018
@wilsonge
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 This Pull Request is Ready To Commit label Dec 14, 2018
@wilsonge wilsonge added this to the Joomla 4.0 milestone Dec 14, 2018
@richard67 richard67 deleted the remove-menu-class-nav-pills branch December 14, 2018 19:12
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.

None yet

6 participants