-
Notifications
You must be signed in to change notification settings - Fork 5
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
FS-9 Mess around with layout (proof of concept) #74
base: 3.x
Are you sure you want to change the base?
Conversation
I added a couple more variables to change the ordering:
|
I added a couple variables to reverse the order between the aside and main column with separate controls on desktop and mobile. I added a flex display on mobile for that. I'd like to play around with this further where we can change the ordering in a more granular way. Stepping back a bit, we are making it so there are some layout controls, which could be nice as module layout settings for site builders to adjust. I like the idea of being able to add custom classes too, and that might be a better custom theming feature. |
…-search-react into test-layout-things
src/components/_fs-aside.scss
Outdated
@@ -6,15 +6,18 @@ | |||
*/ | |||
|
|||
.fs-aside { | |||
width: 22.85714%; |
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.
Not sure how these specific widths were calculated before but we should have grid-template-columns in .fs-container
match them by default to keep the layout the same by default.
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.
22.85714% 74.28571%
We should update src/.env.local.js.example to show all the new variables. |
@@ -100,12 +100,13 @@ class FederatedSolrFacetedSearch extends React.Component { | |||
|
|||
return ( | |||
<LiveAnnouncer> | |||
<div className="fs-container"> | |||
<aside className="fs-aside"> | |||
<div className={`fs-container ${this.props.options.layoutAndClasses.containerClass}`} style={{ gridTemplateColumns: this.props.options.layoutAndClasses.layout }}> |
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.
We need checks if the the following exist before we use them:
this.props.options.layoutAndClasses.containerClass
this.props.options.layoutAndClasses.layout
this.props.options.layoutAndClasses.asideClass
this.props.options.layoutAndClasses.mainClass
We could use the ternary check like we have on this.props.options.layoutAndClasses.reverseDesktopColumns to be consistent.
@nstriedinger this is back to you. |
@@ -46,6 +46,20 @@ module.exports = { | |||
pageTitle: null, | |||
// OPTIONAL: The hostname to emulate when testing. | |||
hostname: "example.local", | |||
layoutAndClasses : { | |||
// OPTIONAL: Specify the column widths. | |||
gridTemplateColumns: "33% 66%", |
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.
this doesn't exactly work with clean percentages because the gap width needs to be subtracted, so we might want to make some preset styles with the widths calculated for the most common percentages. Or use padding instead of the grid gap...
@agentrickard this is still ready to review as proof of concept, but we should tweak the column widths a bit before merging as noted in the recent comment. |
@@ -98,14 +98,25 @@ class FederatedSolrFacetedSearch extends React.Component { | |||
pageTitle = <h1>{this.props.options.pageTitle}</h1>; | |||
} | |||
|
|||
// Grab env vars. | |||
const { | |||
containerClass = '', |
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.
Did we lose some default values here?
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.
The optional custom classes would be additional to existing classes if needed so the default is to have none, so I think this is ok as is.
@@ -10,6 +10,70 @@ | |||
margin: rhythm(1) 0; | |||
} | |||
|
|||
.fs-search-scope__filter { |
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.
@nstriedinger When I was reviewing to see if padding and other previous styling was getting lost I caught these styles from #70 and adding them back in without singularity. Let me know if we decided this styling was not needed. It looks like they may have been removed accidentally.
@@ -8,6 +8,17 @@ | |||
|
|||
.fs-search-form__autocomplete-container { | |||
display: flex; | |||
@include breakpoint($bp1) { |
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.
@nstriedinger these styles as well were removed in pr #70.
@nstriedinger I made this so the default padding if no layout customizations are made matches the old styles, but if the layout is customized I have an em value, because the default percentage looks too wide in some configurations. Take a look and see if this seems reasonable to you. Since there were some general concerns about changes in padding etc, I also went over some of our recent PRs where we deleted a lot of styles and restored a few styles where I think too much got removed. I may try to use our diffy account to compare this to before all the CSS changes to see if there are any remaining differences. |
I feel like we might be going back and forth a bit between keeping CSS bare-bones vs preserving exact previous appearance from 1.x. Maybe we can use https://palantir.atlassian.net/browse/FS-47 to sort this out where we can preserve a lot of previous theming but allow that theming to be disabled as libraries by component. |
Proof of concept for FS-9
Add this to env.local.js
Note that