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

fix: (Core) refactor Layout Grid with latest changes from fundamental-styles #3640

Merged
merged 20 commits into from
Oct 27, 2020

Conversation

mikerodonnell89
Copy link
Member

@mikerodonnell89 mikerodonnell89 commented Oct 16, 2020

Layout grid has received a significant refactor with many breaking changes. Note most code in this PR was done by @salarenko and I just wrote tests/docs

Fundamental Styles issues found during implementation:

BREAKING CHANGE:
The layout grid is no longer a series of components but rather directives. Here are a few other breaking changes:

  • gapSize input replaced with fdLayoutGridColOffset
  • nogap replaced with noGap
  • responsiveness is achieved by setting individual sizes for each of the following: fdLayoutGridCol fdLayoutGridColMd fdLayoutGridColLg fdLayoutGridColXl
  • individual rows can be set with the fdLayoutGridRow directive.

@netlify
Copy link

netlify bot commented Oct 16, 2020

Deploy preview for fundamental-ngx ready!

Built with commit fa417c0

https://deploy-preview-3640--fundamental-ngx.netlify.app

@mikerodonnell89 mikerodonnell89 changed the title [WIP] fix: layout panel [WIP] fix: layout grid Oct 16, 2020
@mikerodonnell89 mikerodonnell89 changed the title [WIP] fix: layout grid fix: layout grid Oct 16, 2020
@mikerodonnell89 mikerodonnell89 requested a review from a team October 16, 2020 22:39
@InnaAtanasova InnaAtanasova changed the title fix: layout grid fix: (Core) refactor Layout Grid with latest changes from fundamental-styles Oct 19, 2020
@droshev droshev added this to In progress in Development via automation Oct 19, 2020
@droshev droshev added this to the Sprint 48 - Bora Bora milestone Oct 19, 2020
@stefanoScalzo
Copy link
Contributor

stefanoScalzo commented Oct 19, 2020

We need examples of the following

  • Responsive columns and offsets

  • Nesting

  • Auto adjusting

@salarenko
Copy link
Contributor

I've introduces some changes:

  • Moved responsive columns and responsive offsets to separate examples for better readability.
  • Moved the logic used to apply .fd-col class to LayoutGridColDirective only.
  • Commented out the API responsible for auto-adjustment behavior which is not available yet.

@stefanoScalzo

  • Auto adjusting is not available in currently used Fundamental Styles version
  • The other examples have been added to the documentation

@mikerodonnell89

  • The documentation is a bit poor. It lacks in information how grid actually behaves, how grid elements should be used, what are the breaking points and many more etc. Could you improve it a bit.
  • Did you document the breaking changes? I don't see them described in the PR.

@stefanoScalzo
Copy link
Contributor

We still need to improve the documentation,
We need to define what the numbers correspond to
What about having a config being passed instead of having a bunch of inputs?
Nesting example: "Add a row" not "Add an row"

@salarenko
Copy link
Contributor

@stefanoScalzo I also thought about the "config" approach, but in this case I find the directives much easier to use and read. Anyway I'm open for discussion, we can talk this through during one of the meetings.

@SAP SAP deleted a comment from netlify bot Oct 20, 2020
Copy link
Contributor

@JKMarkowski JKMarkowski left a comment

Choose a reason for hiding this comment

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

There is 1 minor thing. I will continue review, after changing directives to inputs, as we discussed

@salarenko
Copy link
Contributor

salarenko commented Oct 21, 2020

@stefanoScalzo @JKMarkowski
I've introduced changes according to our latest agreements:

  • Abandon separate directives approach and introduce one col directive with inputs
  • Implement CssClassBuilder
  • Fixed documentation examples

Current markup:

Columns with breakpoints:

    <div fdLayoutGridCol="12" colMd="6" colLg="4" colXl="3"></div>

Offset with breakpoints:

    <div fdLayoutGridCol="4" colOffsetMd="1" colOffsetLg="2" colOffsetXl="4">

Copy link
Contributor

@JKMarkowski JKMarkowski left a comment

Choose a reason for hiding this comment

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

Great Job! I left 2 minor comments, but the rest LGTM

@stefanoScalzo
Copy link
Contributor

The row example should come after before the column example
Nesting columns should be before responsive columns
Explain how to use responsive offset
State breakpoints for responsiveness

@mikerodonnell89
Copy link
Member Author

mikerodonnell89 commented Oct 21, 2020

The row example should come after before the column example
Nesting columns should be before responsive columns

Why?

Explain how to use responsive offset

The responsiveness and offsets work together without any additional effort

Hmm. you should be able to set them independently from each other

@InnaAtanasova InnaAtanasova self-requested a review October 22, 2020 17:25
@salarenko salarenko added the core Core library specific issues label Oct 23, 2020
@salarenko salarenko added 0.24.0 and removed 0.23.0 labels Oct 26, 2020
@mikerodonnell89 mikerodonnell89 merged commit 839eb0a into master Oct 27, 2020
Development automation moved this from In progress to Done Oct 27, 2020
@mikerodonnell89 mikerodonnell89 deleted the fix/3420-layout-panel branch October 27, 2020 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code refactoring core Core library specific issues denoland
Projects
No open projects
Development
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants