-
Notifications
You must be signed in to change notification settings - Fork 6
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
Open filters on page load #3131
Conversation
5dc1cca
to
1dce621
Compare
076484b
to
0020e56
Compare
0020e56
to
1fd4dda
Compare
1fd4dda
to
e57a0ff
Compare
e57a0ff
to
9588671
Compare
9588671
to
bad03ea
Compare
193e2a4
to
f03b959
Compare
f03b959
to
8fbc5f3
Compare
1a6cda3
to
8f5e46b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me overall, just one small comment.
@@ -84,6 +75,22 @@ window.GOVUK.Modules = window.GOVUK.Modules || {}; | |||
} | |||
} | |||
|
|||
OptionSelect.prototype.toggleVisibility = function (isDesktop) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a small nitpick, but I wonder if we should change isDesktop
to isMobile
and then update the logic within the method.
The reason for this suggestion is that the Design System grid for desktop has a min-width of 769px
, so I think we are actually checking for Tablet or Desktop - https://github.com/alphagov/govuk-frontend/blob/87073ebbf6d10f5af1af6c28886cf83c9fd71980/packages/govuk-frontend/src/govuk/settings/_media-queries.scss#L13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. The only problem is i'd have to reverse the logic which I think it might make it harder to read - not sure if you agree though? e.g.
if (!this.mq.matches) {
this.toggleVisibility(true)
} else {
this.toggleVisibility(false)
}
OptionSelect.prototype.toggleVisibility = function (isMobile) {
if (isMobile) {
if (this.isClosedOnLoadMobile === 'true' || this.isClosedOnLoad === 'true') {
this.close()
} else {
this.setContainerHeight(201)
}
} else {
if (this.isClosedOnLoad === 'true') {
this.close()
} else {
this.setupHeight()
}
}
}
An alternative could be rename the variable to something like isTabletOrLarger
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good point, I do like isTabletOrLarger
, happy to go with that 👍
8f5e46b
to
8d7828b
Compare
8d7828b
to
75f8f41
Compare
What
https://trello.com/c/p6FCqlHs/2042-expand-filter-and-location-facets-by-default-on-mobile
Add
open_on_load
property to open filters, on page load, on mobile. Configured for Find a licence (specialist finder) only.Why
During user research, a common observation has emerged that mobile users do not interact with the 'Filter' button.
Visual changes
Before (Find a licence)
After (Find a licence)
Anything else
alphagov/publishing-api#2471
alphagov/specialist-publisher#2369