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

Add tabs component #776

Merged
merged 6 commits into from
Jun 20, 2018
Merged

Add tabs component #776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b29b698
Add tabs component
alex-ju Jun 5, 2018
f5d1af4
Add readme for tabs component
alex-ju Jun 5, 2018
a5ccae8
Add JavaScript for tabs component
alex-ju Jun 6, 2018
de88b1d
Add classList polyfill
alex-ju Jun 7, 2018
22edd64
Import tabs component
alex-ju Jun 5, 2018
7fe359e
Update changelog
alex-ju Jun 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@ Note: We're not following semantic versioning yet, we are going to talk about th

- Add default text for back-link component
([PR #793](https://github.com/alphagov/govuk-frontend/pull/793))

- Add default container class to the header component
([PR #807](https://github.com/alphagov/govuk-frontend/pull/807))

- Add tabs component – thanks to [@adamsilver](https://github.com/adamsilver) and [@trevorsaint](https://github.com/trevorsaint) for contributing
([PR #776](https://github.com/alphagov/govuk-frontend/pull/776))

🔧 Fixes:

- Pull Request Title goes here
Expand Down
9 changes: 8 additions & 1 deletion src/all.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import Checkboxes from './components/checkboxes/checkboxes'
import ErrorSummary from './components/error-summary/error-summary'
import Header from './components/header/header'
import Radios from './components/radios/radios'
import Tabs from './components/tabs/tabs'

function initAll () {
new Button(document).init()
Expand All @@ -31,6 +32,11 @@ function initAll () {
nodeListForEach($radios, function ($radio) {
new Radios($radio).init()
})

var $tabs = document.querySelectorAll('[data-module="tabs"]')
Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment the macro template has the data attribute on the container. This means that if I use the macro, it will automatically get instantiated into tabs.

This could become problematic if I want to do something more custom. Like listen to an event fired by the instance (I know we can't do that right now).

Is GOV.UK Frontend continuing to instantiate automatically like this?

Copy link
Contributor Author

@alex-ju alex-ju Jun 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Your concern is valid, but this is consistent with how the other components are being initialised. Will make a note and look into that for all the components.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, it might be okay, broadly speaking, for say tabs and conditional radio button reveals. But as a general approach it can be overly constraining.

Thanks @alex-ju

Copy link
Contributor

Choose a reason for hiding this comment

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

This use case is already accounted for: you can avoid initAll() and call the constructor yourself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we did work to stop initAll() from executing automatically :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, not initialising or initialising separately a specific component is also an option.
When Adam mentioned 'something custom', I was thinking of a case when you want to initialise but hook different listeners or add custom functionality to a Tabs instance before doing that, which I think is still possible with the current model:

var myTabs = new Tabs($tabs)
myTabs.myCustomFunction = function () { ... }
myTabs.init()

nodeListForEach($tabs, function ($tabs) {
new Tabs($tabs).init()
})
}

export {
Expand All @@ -40,5 +46,6 @@ export {
Checkboxes,
ErrorSummary,
Header,
Radios
Radios,
Tabs
}
3 changes: 2 additions & 1 deletion src/all.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ describe('GOV.UK Frontend', () => {
'Checkboxes',
'ErrorSummary',
'Header',
'Radios'
'Radios',
'Tabs'
])
})
it('exported Components can be initialised', async () => {
Expand Down
1 change: 1 addition & 0 deletions src/components/_all.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
@import "label/label";
@import "panel/panel";
@import "phase-banner/phase-banner";
@import "tabs/tabs";
@import "tag/tag";
@import "radios/radios";
@import "select/select";
Expand Down
Loading