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.

Sign up for GitHub

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

Jump to bottom

FRONT-2481: Refactor Nav & Tab pattern #74

Merged
merged 15 commits into from
Dec 9, 2021
Merged

FRONT-2481: Refactor Nav & Tab pattern #74

merged 15 commits into from
Dec 9, 2021

Conversation

abel-santos-corral
Copy link
Contributor

Jira issue(s):

@abel-santos-corral abel-santos-corral linked an issue Sep 22, 2021 that may be closed by this pull request
Refactor Nav & Tabs pattern with BCL component #25
Closed
abel-santos-corral
abel-santos-corral commented Oct 25, 2021
View reviewed changes
tests/fixtures/markup_rendering_pattern.yml Outdated
@@ -42,3 +42,146 @@ button:
'button svg': 1
contains:
'button.btn-secondary': "Button label"
nav_with_ul:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, adjust the titles to the names at netlify. Here use nav_tabs

tests/fixtures/markup_rendering_pattern.yml Outdated
pills: true
tabs: true
vertical: false
full_width: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This setting is good, but add please an assertion that class nav-fill is added at ul (check in netlify to see it working).

tests/fixtures/markup_rendering_pattern.yml Outdated
'#id': nav
'#fields':
settings:
pills: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Delete this setting. Check it at netlify: there, at data and also at the Controls, no pills are set for the Tabs example.

tests/fixtures/markup_rendering_pattern.yml Outdated
settings:
pills: true
tabs: true
vertical: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same for vertical, this setting can be removed from here. As per netlify.

tests/fixtures/markup_rendering_pattern.yml Outdated
id: "fourth-tab"
target: "fourth"
assertions:
count:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add assertion for the nav-fill at ul tag. See in HTML tab of netlify of Tabs navigation.

tests/fixtures/markup_rendering_pattern.yml Outdated
'div[aria-labelledby="second-tab"]': "This is a demo content for the Second Tab. Lorem ipsum dolor sit amet, consectetur adipiscing elit."
'div[aria-labelledby="third-tab"]': "This is a demo content for the Third Tab. Lorem ipsum dolor sit amet, consectetur adipiscing elit."
'div[aria-labelledby="fourth-tab"]': "This is a demo content for the Fourth Tab. Lorem ipsum dolor sit amet, consectetur adipiscing elit."
nav_vertical:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rename it as nav_default_vertical to be coincident with the data at netlify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also copy the test and repeat it as nav_default without the vertical testing. Adapt the assertions to the default testing.

tests/fixtures/markup_rendering_pattern.yml Outdated
Comment on lines 125 to 132
pills: false
tabs: false
vertical: true
full_width: true
alignment: end
tabs_content: false
nav: true
navbar: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check at netlify, only is necessary pills, nav and vertical. Other settings can be deleted from here.

drishu
drishu requested changes Nov 28, 2021
View reviewed changes
tests/fixtures/markup_rendering_patterns/nav.yml Outdated
@@ -0,0 +1,516 @@
navigation_default_full_width_with_pills_horizontal:
Copy link
Contributor

Choose a reason for hiding this comment

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

the location of this folder changed

drishu
drishu previously approved these changes Nov 28, 2021
View reviewed changes
escuriola
escuriola previously approved these changes Nov 29, 2021
View reviewed changes
@drishu drishu dismissed stale reviews from escuriola and themself via ee25ad4 November 30, 2021 08:33
drishu
drishu requested changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@drishu drishu left a comment

Choose a reason for hiding this comment

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

The dropdown does not work, add a submenu item and it should show a dropdown

drishu
drishu requested changes Dec 3, 2021
View reviewed changes
Copy link
Contributor

@drishu drishu left a comment

Choose a reason for hiding this comment

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

I can't review the rendering part because I can't revisit netlify bcl 0.14, so it will have to do for now, and this will have to be looked into for the next bcl upgrade

includes/menu.inc Outdated Show resolved Hide resolved
includes/menu.inc Outdated Show resolved Hide resolved
includes/menu.inc Outdated Show resolved Hide resolved
includes/menu.inc Outdated Show resolved Hide resolved
includes/menu.inc Outdated Show resolved Hide resolved
drishu
drishu requested changes Dec 8, 2021
View reviewed changes
templates/patterns/nav/pattern-nav--variant-custom_tabs.html.twig Outdated Show resolved Hide resolved
templates/patterns/nav/nav.ui_patterns.yml Outdated Show resolved Hide resolved
includes/menu.inc Outdated Show resolved Hide resolved
includes/menu.inc Outdated Show resolved Hide resolved
includes/menu.inc Outdated Show resolved Hide resolved
@drishu drishu merged commit 6ecd84b into 1.x Dec 9, 2021
@drishu drishu deleted the FRONT-2481 branch December 9, 2021 11:10
@dxvargas dxvargas mentioned this pull request Jan 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@escuriola escuriola escuriola approved these changes

@drishu drishu drishu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor Nav & Tabs pattern with BCL component
3 participants
@abel-santos-corral @escuriola @drishu