Ported fluid grid component from stache2 #957
Conversation
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: f5c0b1d (Please note that this is a fully automated comment.) |
Codecov Report
@@ Coverage Diff @@
## master #957 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 304 307 +3
Lines 5545 5573 +28
Branches 698 701 +3
=====================================
+ Hits 5545 5573 +28
Continue to review full report at Codecov.
|
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 5468021 (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 793ec43 (Please note that this is a fully automated comment.) |
Blackbaud-PatrickOFriel
requested review from
Blackbaud-PatrickOFriel and
Blackbaud-PaulCrowder
| @@ -0,0 +1,392 @@ | |||
| <sky-demo-nav></sky-demo-nav> | |||
There was a problem hiding this comment.
In this file, instead of putting all this code inline, create a component like in alert and other component documentation and reference in the index.html.
| Demo | ||
| </h2> | ||
|
|
||
| <style> |
There was a problem hiding this comment.
When you move these examples into a separate component, these styles can be part of the component instead of inline
src/modules/fluid-grid/index.ts
Outdated
| @@ -0,0 +1,3 @@ | |||
| export * from './column.component'; | |||
There was a problem hiding this comment.
Don't use the * here because it will sometimes mess with AOT compilation. Instead call out the specific things you're exporting
Blackbaud-PatrickOFriel
requested review from
blackbaud-johnly and
Blackbaud-ToddRoberts
|
Thanks @Blackbaud-PatrickOFriel! Is there a way to see if the plunkr works ahead of this component being released? Additionally, what all do I need to do in order to make sure the screenshots for the visual tests were generated correctly? |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 6dc0ad1 (Please note that this is a fully automated comment.) |
|
The plunker is tough to verify ahead of time because we load it from the npm CDN, you would have to essentially serve the skyux bundle up on localhost and point the plunker to that instead of the npm cdn to verify that it works. Visual tests are created when you merge to master, but you can run them locally are your machine by following the guidelines here to get an idea of what they'll look like. (You can also run just your tests by putting an |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 498217c (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 2525240 (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 3e56d6f (Please note that this is a fully automated comment.) |
src/core.ts
Outdated
| @@ -64,6 +65,7 @@ import { SkyWaitModule } from './modules/wait'; | |||
| SkyErrorModule, | |||
| SkyFileAttachmentsModule, | |||
| SkyFilterModule, | |||
| SkyFluidGridModule, | |||
| SkyGridModule, | |||
| SkyKeyInfoModule, | |||
| SkyLabelModule, | |||
There was a problem hiding this comment.
Here, also export the module and components so that consumers have access through core. You can see examples below
|
That's a good catch, I'd accidentally used row in place of column at the end of the summary - sorry for the confusion there. I've also added the pixel ranges to each of the size properties. |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: fd9a31d (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 02729f1 (Please note that this is a fully automated comment.) |
| @@ -317,6 +317,27 @@ export class SkyDemoComponentsService { | |||
| } | |||
| }, | |||
| { | |||
| name: 'Fluid Grid', | |||
There was a problem hiding this comment.
Is this name property displayed as a header anywhere in the docs? If so, "G" should be lowercase.
| name: 'Fluid Grid', | ||
| icon: 'table', | ||
| // tslint:disable-next-line | ||
| summary: `Provides a layout grid for creating responsive content.`, |
There was a problem hiding this comment.
Not sure if this summary property is displayed anywhere in the docs, but do we need "creating" here? I mean, does the grid actually create responsive content? Or does it just display responsive content?
| @@ -0,0 +1,29 @@ | |||
| <sky-demo-page title="Fluid Grid"> | |||
| <sky-demo-page-summary> | |||
| The Grid components provide a 12 column layout grid for organizing content for all device sizes. The grid consists of <code>sky-row</code> and <code>sky-column</code> tags. Every column must be contained in a row. Each column can have multiple properties specified in order to handle different behavior for each screen size. Below the width of 768px (small screen) all elements will be displayed in a single column. | |||
There was a problem hiding this comment.
Shouldn't this be "The fluid grid component provides" instead of "The Grid components provide"? This is adding a separate grid component, right? So I assume it needs a different name than the existing grid component. And I think it should also be "component" singular in the docs. (Either that or I think it should be "modules" if it needs to be plural.)
Also in the first sentence, hyphenate "12-column." And can "layout grid" just be "layout"? I'd also change "for organizing" to "to organize." And should we include something about responsive content since that's what we emphasized in the previous description? Maybe just add" responsive before "content"?
The fourth sentence seems awkwardly phrased to me. Like maybe we're saying something slightly different than we mean. I mean, when I look at the regular grid component, the columns in the example have multiple properties, so I don't think we mean to stress that columns can have multiple properties. I think that we mean columns can have multiple instances of the same property with each instance targeted to different screen sizes. Is that right? If so, maybe something like "Each column can include multiple instances of properties to specify behavior for different screen sizes." Is that accurate? If not, can we just say "Each column can specify behavior for different screen sizes."?
In the last sentence, I'd flip the start to "On small screens (less than 768px)" and add a comma after the closing parenthesis. And to use active voice, I'd also change "all elements will be displayed" to "the grid displays all elements."
|
|
||
| <sky-demo-page-properties sectionHeading="Sky row properties"> | ||
| <sky-demo-page-property propertyName="reverseColumnOrder" defaultValue="false" isOptional="true"> | ||
| Reverses the display order for the child columns when set to <code>true</code>. |
There was a problem hiding this comment.
What designates a column as a child column? I feel like I'm missing something here. I thought this property would change the order (from left to right) of the columns in the grid so that whatever would have been last (far right) will be first (far left) with the other columns changing likewise (with the middle column staying right where it was if you have an odd number of columns). But the fact that we are referring to child columns makes me wonder if this property applies to some subset of the columns or if I'm just not following this at all.
Also, how is the original display order determined? Is there a default order? If so, should we specify that here? If not, should we explain how the unreversed display order is determined?
On a more minor wording note, I'd tweak the description slightly to follow the wording pattern we usually use with Boolean properties: "Indicates whether to reverse the display for for the child columns."
| </sky-demo-page-properties> | ||
| <sky-demo-page-properties sectionHeading="Sky column properties"> | ||
| <sky-demo-page-property propertyName="screenSmall" isOptional="true"> | ||
| The number of columns (1-12) the element will use on a small screen (768-991px). |
There was a problem hiding this comment.
I'd start with "Specifies the" and add "for" after "(1-12)" and change "will" to "to."
Also, I thought that above description said small screens were less than 768px. Should the previous item say extra small screens?
| The number of columns (1-12) the element will use on a small screen (768-991px). | ||
| </sky-demo-page-property> | ||
| <sky-demo-page-property propertyName="screenMedium" isOptional="true"> | ||
| The number of columns (1-12) the element will use on a medium screen (992-1199px). |
There was a problem hiding this comment.
I'd start with "Specifies the" and add "for" after "(1-12)" and change "will" to "to."
| The number of columns (1-12) the element will use on a medium screen (992-1199px). | ||
| </sky-demo-page-property> | ||
| <sky-demo-page-property propertyName="screenLarge" isOptional="true"> | ||
| The number of columns (1-12) the element will use on a large screen (1200px+). |
There was a problem hiding this comment.
I'd start with "Specifies the" and add "for" after "(1-12)" and change "will" to "to."
|
@blackbaud-johnly I've pushed an update with changes to the wording, but let's definitely sync up at some point so we can make sure we're both on the same page on the properties and how best to describe them. |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 24bb453 (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 5899762 (Please note that this is a fully automated comment.) |
|
Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 268cd57 (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: e509a85 (Please note that this is a fully automated comment.) |
| name: 'Fluid grid', | ||
| icon: 'table', | ||
| // tslint:disable-next-line | ||
| summary: `Provides a layout grid for displaying responsive content.`, |
There was a problem hiding this comment.
I'd change this to "Provides a responsive 12-column layout to organize content for all device sizes."
| @@ -0,0 +1,29 @@ | |||
| <sky-demo-page title="Fluid grid"> | |||
| <sky-demo-page-summary> | |||
| The fluid grid component provides a 12-column layout to organize responsive content for all device sizes. The grid consists of <code>sky-row</code> and <code>sky-column</code> tags. Every column must be contained in a row. Each column can specify behavior for different screen sizes. On extra small screens (less than 768px) the grid displays all elements in a single column. | |||
There was a problem hiding this comment.
I'd change this to "The fluid grid component provides a responsive 12-column layout to organize content for all device sizes. The fluid grid consists of sky-row and sky-column tags. You place column elements in row elements, and then you can specify how much of the layout to use for each column element on small, medium, and large screens. For extra small screens (less than 768px), the fluid grid automatically stacks columns vertically across the entire 12-column layout."
| </sky-demo-page-properties> | ||
| <sky-demo-page-properties sectionHeading="Sky column properties"> | ||
| <sky-demo-page-property propertyName="screenSmall" isOptional="true"> | ||
| Specifies the number of columns (1-12) for the element to use on a small screen (768-991px). |
There was a problem hiding this comment.
I'd change this to "Specifies the number of columns (1-12) to use for the element on small screens (768-991px). If you do not specify a value, the fluid grid displays the element vertically across the entire layout."
And if we need to call out the recommendation that we discussed about setting screenSmall only, we could add "To use the same value for small, medium and large screens, we recommend setting the screenSmall only." But I don't really think that's necessary.
| Specifies the number of columns (1-12) for the element to use on a small screen (768-991px). | ||
| </sky-demo-page-property> | ||
| <sky-demo-page-property propertyName="screenMedium" isOptional="true"> | ||
| Specifies the number of columns (1-12) for the element to use on a medium screen (992-1199px). |
There was a problem hiding this comment.
I'd change this to "Specifies the number of columns (1-12) to use for the element on medium screens (992-1199px). If you do not specify a value, the element inherits the screenSmall value."
| Specifies the number of columns (1-12) for the element to use on a medium screen (992-1199px). | ||
| </sky-demo-page-property> | ||
| <sky-demo-page-property propertyName="screenLarge" isOptional="true"> | ||
| Specifies the number of columns (1-12) for the element to use on a large screen (greater than 1200px). |
There was a problem hiding this comment.
I'd change this to "Specifies the number of columns (1-12) to use for the element on large screens (greater than 1200px). If you do not specify a value, the element inherits the screenMedium value."
|
|
||
| <sky-demo-page-properties sectionHeading="Sky row properties"> | ||
| <sky-demo-page-property propertyName="reverseColumnOrder" defaultValue="false" isOptional="true"> | ||
| Indicates whether to reverse the display order for columns within this row. |
There was a problem hiding this comment.
I'd change "within this" to "in the."
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 165459d (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 5f96c39 (Please note that this is a fully automated comment.) |
|
Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: 854e600 (Please note that this is a fully automated comment.) |
Direct port of the stache2 grid component written by @Blackbaud-SteveBrush. Talking with him, we decided to leave out the InputConverter in use on the sky-row component for the sake of consistency with existing SkyUX2 components. Added some basic visual tests and updated some of the wording on the documentation/demo.