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.

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

Tag grid #298

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Tag grid #298

wants to merge 10 commits into from

Conversation

pgregorova
Copy link
Member

added Grid tag with few basic examples that we tend to use a lot on projects, mainly for smaller components. Maybe how we used to use 2,3,4-up, we can now replace with auto-fit or auto-fill.

This is literally just a starting point as with CSS grid we can do so much when it comes to layout. It would be nice to be able to show on this page all the different ways we could modify simple layout and components.

I started with flexbox fallbacks for the basic grid items, which eventually we will be able to eliminate, once we won't have to support anything below Edge (which may be a while still)

@pgregorova pgregorova changed the base branch from master to develop December 1, 2017 19:12
Copy link
Member

@drolsen drolsen left a comment

Choose a reason for hiding this comment

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

This is really great work Petra!

Only change I'd like to see is if we can't simplify the Grid.css up a bit with a smaller loop, and possibly break out options into a grids variable file or :root of grids.css.

}
}

@media (--medium) {
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding:
https://www.npmjs.com/package/postcss-for
https://www.npmjs.com/package/postcss-at-rules-variables

To sum up all these column rules into a loop and also a grids variable file?

	@for $i from 1 to var(--grid--column--count) {
		&--span-$i {
			flex-basis: calc(calc(calc($i/var(--grid--column--count)) * 100%) - var(--grid--gap));
			width: calc(calc(calc($i/var(--grid--column--count)) * 100%) - var(--grid--gap));
		}

		&--span-$i {
			grid-column: auto / span $i;
		}
	}

Petra Gregorová added 3 commits December 6, 2017 14:44
@drolsen
Copy link
Member

drolsen commented Jan 10, 2018

I would like to get some more opinions on this PR when everyone has a sec.

  1. I personally find value in having a grid system baslined in our framework, however I know and understand why others feel a grid system should only manifest itself as the need arises. Thoughts please?

  2. Should a grid system in FC be element based or plugin based? @pgregorova has written a version that supports flex fallback, but does require both react tag and supporting CSS, while a plugin does not offer flex fallback, but is only CSS authoring based (https://www.npmjs.com/package/postcss-grid). Thoughts please?

Thanks!

@bstaruk
Copy link
Contributor

bstaruk commented Jan 10, 2018

Almost every website I've built in recent memory has utilized a grid of some sort, and they are pretty consistent in how they function. The parts that aren't (such as gutters and spacing) can be defined and managed via variables.

I think if there's something that reusable and that config'able, it's a prime candidate for becoming a baseline component/tag.

I wouldn't argue against using a PostCSS plugin to make the tag stylesheet prettier, but it does add another dependency for very little actual gain. I like Petra's syntax just fine :)

So I guess my input here is that I'd love to see a Grid tag become part of FC, but I could go either way on using the postcss-grid plugin to achieve that.

@drolsen
Copy link
Member

drolsen commented Jan 28, 2018

I've made some changes here that are up for a much larger conversation we can have later this week in our FED meeting:

  • Re-based development to align branch to new atomic changes.
  • Included postcss @for loops
  • Reduced grid system down to single breakpoint (now defined in variables/breakpoints.css).
  • Relocated grid variables into variables/grid.css.
  • Relocated flex fallback for IE into optional GridIE.css file.
  • Added column sorting (works in IE10+)
  • [Question to group] Is auto-fill and auto-fix worth baseline support if we have to bake in a fixed minmax value, or should we remove it?
  • Removed nth-up abilities as it will be part of a future Nup atom coming soon.

Thanks!

@@ -0,0 +1,47 @@
.Grid {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use some documentation here explaining what this is and when it is used (because looking at this at the moment, it looks like both 'Grid.css' and 'GridIE.css' are being bundled and applied. It would be nice if we could have some Javascript look at the browser being used, and apply the correct styles when it needs to? Cleaner solution but a bit more work. Open for discussion.

@@ -0,0 +1,72 @@
export const Grid = (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

'Grid' can be shorthanded to a 'FcCreateBasicComponent' instance.

@@ -0,0 +1,56 @@
.Grid {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use some documentation either here, or in the README, how we expect our formatting of 'postcss-at-rules' and 'postcss-for' plugins to be. We should also discuss as a group what that looks like, because we haven't used these in FC as of yet.

@@ -0,0 +1,24 @@
# Grid

Copy link
Contributor

Choose a reason for hiding this comment

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

README could use explicit language as to what Grid is. (makes Grid consistent with all other Baseline components).

@CSKingMartin
Copy link
Contributor

Note: when I pull this locally there also appears to be a hanging 'yarn.lock' file that isn't committed. I'm hesitant to just commit mine - please double check. If I remember right, 'yarn.lock' files should always be committed?

@elseloop elseloop removed their request for review February 5, 2018 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants