feat: adds no wrap demo, refactors Y margins #22

MickL · 2021-01-24T15:36:19Z

updates demo index.html with a no wrap and no animation placeholder variation
refactos Y margin's that works for both variations, the no wrapper and wrapped one
extracts $ph-border var
updates readme
gives the loading animation in ::before pointer-events: none so it is not annoying when trying to debug with the inspector

LE:

~~Added .clean class to wrapper to have no background, border, margin and padding~~ replaced by first line in the description
~~The last .ph-row should not have margin-bottom~~ replaced by second line
~~Added uneven cols to have more flexibility~~ we'll add this in a new PR

Fixes #20

zalog · 2021-01-31T17:38:30Z

Hi @MickL, thanks again for your time! :)

If it's ok with you, let's make some changes please:

revert any changes from folder /dist or files packages / packages-lock; this will come from release branch later, it's a git-flow pipe :)
rebase branch
revert modification from cols. Please let's try to keep grid output as low as possible for now. If this is requested by our community again, we can reconsider it then :)
please, let's follow this commit message format
please try atomic commits, like modifications from README in a separated commit

Nice touch about pointer-events.

DKhalil · 2021-01-31T17:43:46Z

revert modification from cols. Please let's try to keep grid output as low as possible for now. If this is requested by our community again, we can reconsider it then :)

Just to add my 2 cents, but I'd really like to see more column options as well! But you could consider adding a configuration setting like in bootstrap, if you don't want to unnecessarily bloat up the final css file:

https://github.com/twbs/bootstrap/blob/e79c8f3489527d7f5eef2bb3cf14856f26c49871/scss/_variables.scss#L332-L334
https://github.com/twbs/bootstrap/blob/e79c8f3489527d7f5eef2bb3cf14856f26c49871/scss/mixins/_grid.scss#L61

zalog · 2021-01-31T17:42:33Z

src/scss/_layout.scss

@@ -36,6 +37,17 @@
    padding-right: ($ph-gutter / 2);
    padding-left: ($ph-gutter / 2);
  }
+
+  &.clean {


Do you think we should rename .clean in something more specific? .clean. Something like .ph-item-full, .ph-item-border-0, .ph-item-*.

zalog · 2021-01-31T17:51:36Z

src/scss/_layout.scss

@@ -56,23 +68,45 @@
  .empty {
    background-color: rgba($ph-bg, 0);
  }
+
+  &:last-child {


Do you think we should use * + * with margin-top to be less css specificity?

zalog · 2021-01-31T17:54:46Z

src/scss/_variables.scss

@@ -4,7 +4,8 @@ $ph-border-radius:        2px !default;

 $ph-gutter:               30px !default;
 $ph-spacer:               15px !default;
+$ph-item-border:          1px solid darken($ph-bg, 10%) !default;


I think we should rename it to $ph-border and move it on line 4, to match the others. What do you think?

zalog · 2021-01-31T17:57:08Z

"But you could consider adding a configuration setting like in bootstrap" - wooow love it! :)

If you have time for this, please feel free to make a new PR, if not thanks for the idea, we'll do this.

MickL · 2021-01-31T18:27:44Z

Hey guys, will check tomorrow. Just so you know you can directly commit to this PR of you want.

Btw im not super happy that it seema to be required to have an extra col before row and col's

zalog · 2021-01-31T18:41:25Z

"it seema to be required to have an extra col before row and col's" - hmm, I don't think so. Or I don't get it, could you give an example please?

MickL · 2021-02-01T15:29:10Z

I mean that this works:

<div class="ph-item">
        <div class="ph-col-12">
          <div class="ph-row">
            <div class="ph-col-2 empty"></div>
            <div class="ph-col-8"></div>
            <div class="ph-col-2 empty"></div>
          </div>
        </div>
      </div>
</div>

But this does not:

<div class="ph-item">
          <div class="ph-row">
            <div class="ph-col-2 empty"></div>
            <div class="ph-col-8"></div>
            <div class="ph-col-2 empty"></div>
          </div>
      </div>
</div>

Or:

          <div class="ph-row">
            <div class="ph-col-2 empty"></div>
            <div class="ph-col-8"></div>
            <div class="ph-col-2 empty"></div>
          </div>
      </div>

-> There is always an extra ph-col required to wrap which is unnecessary.

zalog · 2021-02-03T18:50:12Z

Hi @MickL any news about code review, do you need some help?

About the wrapper. This PR resets the wrapper that still exists in our markup, so your solution includes it.
I think removing wrapper would be breaking change, so it would be better to keep that in a separated new PR.

What do you think?

MickL · 2021-03-08T10:17:01Z

I didnt had time yet. Guess I will end up rewrite all this code for my project. I dont like that I need so many unnecessary wrappers. And I dont want to refactor all your code, I dont think that you will like it.

You can make changes to this PR directly so feel free to contribute and fix the things mentioned above.

zalog · 2021-03-08T10:27:18Z

Ok @MickL. Thank you very much for your time, contribution and ideas.
Sure, I'll get back here in a few days with some changes and I hope you'll like it.
Keep in touch.
Have a great day.

- adds variable to control wrapper border - adds uneven cols to have more flexibility - adds .clean class to wrapper to have no background, border, margin and padding - last .ph-row should not have margin-bottom - updates readme

This will comes in another PR from configuration setting.

MickL · 2021-03-13T12:58:42Z

Hey, so as I said I ended up writing my own placeholders. But I want to give you what I have learned and what would be useful in your library:

I wanted to place placeholders without any wrapper, just a ph-col-12 without anything else
I added a .ph-row for creating the margin-bottom, so each line of placeholder-text has a row
I named ph-picture -> ph-round
Instead of .big I created a class for each text-element I have. In my case this was regular text, small text .small, h1, h2, h3, h4, h5 and then I do <div class="ph-col-12 h2"></div>:

$ph-height:                   18px;
$ph-height-small:             15px;
$ph-height-h1:                80px;
$ph-height-h2:                58px;
$ph-height-h3:                44px;
$ph-height-h4:                25px;

As @DKhalil proposed I added dynamic creation of rows. For that I used the Bootstrap mixins I already have in my project:
Further I needed to have classes for each breakpoint, same as Bootstrap has. This is what I ended up with:

@each $breakpoint in map-keys($grid-breakpoints) {
  $infix: breakpoint-infix($breakpoint, $grid-breakpoints);

  @include media-breakpoint-up($breakpoint, $grid-breakpoints) {
    @if $grid-row-columns > 0 {
      @for $i from 1 through $ph-grid-columns {
        .ph-col#{$infix}-#{$i} {
          background-color: $ph-color;
          flex:             0 0 auto;
          width:            percentage($i / $ph-grid-columns);
          height:           $ph-height;
        }
      }
    }
  }
}

And then I can do:

<div class="ph-col-6 ph-col-md-2 small"></div>

I needed to remove the animation because it goes visibly over the placeholders. You dont see that in your example because you have a white background, but if the background is something else you will see the animation. I couldnt fix that.

MickL · 2021-03-13T13:00:50Z

Here is my final code

$ph-bg:                       white;
$ph-color:                    $gray-300;
$ph-image-height:             120px;
$ph-height:                   18px;
$ph-height-small:             15px;
$ph-height-h1:                80px;
$ph-height-h2:                58px;
$ph-height-h3:                44px;
$ph-height-h4:                25px;
$ph-grid-columns:             12;
$ph-gutter-width:             $grid-gutter-width/4;
$ph-animation-duration:       .8s;

div[class^="ph-col"],
.ph-image,
.ph-round {
  background-color: $ph-color;
}

.ph-row {
  display:       flex;
  margin-bottom: $ph-gutter-width;

  &.center {
    justify-content: center;
  }

  &.right {
    justify-content: flex-end;
  }
}

.ph-image {
  height: $ph-image-height;
  width:  100%;
}

.ph-round {
  border-radius: 50%;
  width:         $ph-image-height;
  height:        $ph-image-height;
}

div[class^="ph-col"] {
  &.empty {
    background-color: transparent;
  }

  &.small {
    height: $ph-height-small;
  }

  &.h1 {
    height: $ph-height-h1;
  }

  &.h2 {
    height: $ph-height-h2;
  }

  &.h3 {
    height: $ph-height-h3;
  }

  &.h4 {
    height: $ph-height-h4;
  }
}

@each $breakpoint in map-keys($grid-breakpoints) {
  $infix: breakpoint-infix($breakpoint, $grid-breakpoints);

  @include media-breakpoint-up($breakpoint, $grid-breakpoints) {
    @if $grid-row-columns > 0 {
      @for $i from 1 through $ph-grid-columns {
        .ph-col#{$infix}-#{$i} {
          background-color: $ph-color;
          flex:             0 0 auto;
          width:            percentage($i / $ph-grid-columns);
          height:           $ph-height;
        }
      }
    }
  }
}

zalog · 2021-03-14T19:47:38Z

Wow! Thank you! I think you did a great job :)
I'll keep you up2date with what we'll decide here.

Renames var to match project naming.

Fixes bottom symmetry inside `.ph-item`.

- refactors how margins are applied - uses `.ph-col` as wrapper in demo, but all `.ph-col-*` are working for spacings

zalog · 2021-03-16T11:58:35Z

Hi @MickL!

Please take a look, I think we are done.

Changes:

removed class .clean, but added a working example with a clean item in demo page / thank for your idea of using placeholder without any wrapper. I hope you find this a better idea instead of adding a new class :)
refactored margins for both variations, the no wrapper one and the old

Regarding on this type of animation... Sorry, is tightly coupled with background. So if we need to use a transparent background we have to remove it. In the near feature I plan to add a second animation, some simple loading circle :)

I think this is the best we can get with no breaking changes :)
What do you think?

Again, thanks a lot for your time and precious contributions.

LE: please take a look at this PR #25. I think we should enable ltr support also for no wrapped version. Do you have any suggestion on where to move direction css prop?

zalog · 2021-03-21T08:34:23Z

Hi @MickL,

what do you think? Do you have any feedback or suggestions here :)

zalog · 2021-03-26T07:42:30Z

Hi guys. I updated the PR description accordingly with the latest commits :)

DKhalil · 2021-03-26T08:55:44Z

@zalog how are you feeling about the inclusion of the column generation code that @MickL integrated? That way everyone can just configure the amount of columns they'd like to have

MickL · 2021-03-26T08:57:12Z

Cols AND breakpoints are important imo

zalog · 2021-03-26T09:04:10Z

Sure, that's a great idea. Actually I love it :), but we try to keep PR's small and close them fast.
So, @MickL you can open a new branch/PR, or I'll do that and mention you there.
How that's sounds for you guys?

MickL · 2021-03-26T09:37:20Z

I agree to keep PRs small. I will not have the time but I posted my code above that you can copy from. @DKhalil I would open another issue for dynamic cols and breakpoints.

zalog · 2021-03-26T09:51:04Z

Tnks @MickL

Ok, we'll merge this, and I'll keep you guys posted in a future PR.

zalog reviewed Jan 31, 2021

View reviewed changes

zalog force-pushed the wrapper-improvements branch 2 times, most recently from a877ef6 to 401d251 Compare March 13, 2021 09:47

zalog force-pushed the develop branch from 6bc4788 to 4708297 Compare March 13, 2021 11:30

feat: adds wrapper var, .clean, some flexibility

61bc000

- adds variable to control wrapper border - adds uneven cols to have more flexibility - adds .clean class to wrapper to have no background, border, margin and padding - last .ph-row should not have margin-bottom - updates readme

zalog force-pushed the wrapper-improvements branch from 401d251 to 61bc000 Compare March 13, 2021 11:48

revert: removes uneven cols

5747b6d

This will comes in another PR from configuration setting.

zalog added 5 commits March 15, 2021 08:12

feat: renames $ph-border var

0cdada7

Renames var to match project naming.

fix: removes not needed margin reset

62a1e3f

Fixes bottom symmetry inside `.ph-item`.

feat: adds empty, no wrap placeholder

adcff3d

feat(demo): adds some headings styling

1f62f67

refactor: moves margins for markup flexibility

21f0f1a

- refactors how margins are applied - uses `.ph-col` as wrapper in demo, but all `.ph-col-*` are working for spacings

zalog requested a review from nagnibedamihai March 23, 2021 05:55

zalog self-assigned this Mar 23, 2021

zalog changed the title ~~Improvements to wrapper and col's~~ feat: adds no wrap demo, refactors Y margins Mar 27, 2021

zalog requested review from zalog and removed request for nagnibedamihai March 27, 2021 07:22

zalog merged commit 792dcc6 into zalog:develop Mar 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adds no wrap demo, refactors Y margins #22

feat: adds no wrap demo, refactors Y margins #22

MickL commented Jan 24, 2021 •

edited by zalog

Loading

zalog commented Jan 31, 2021 •

edited

Loading

DKhalil commented Jan 31, 2021 •

edited

Loading

zalog Jan 31, 2021

zalog Jan 31, 2021 •

edited

Loading

zalog Jan 31, 2021

zalog commented Jan 31, 2021 •

edited

Loading

MickL commented Jan 31, 2021 •

edited

Loading

zalog commented Jan 31, 2021 •

edited

Loading

MickL commented Feb 1, 2021 •

edited

Loading

zalog commented Feb 3, 2021

MickL commented Mar 8, 2021

zalog commented Mar 8, 2021

MickL commented Mar 13, 2021 •

edited

Loading

MickL commented Mar 13, 2021

zalog commented Mar 14, 2021 •

edited

Loading

zalog commented Mar 16, 2021 •

edited

Loading

zalog commented Mar 21, 2021

zalog commented Mar 26, 2021

DKhalil commented Mar 26, 2021

MickL commented Mar 26, 2021

zalog commented Mar 26, 2021 •

edited

Loading

MickL commented Mar 26, 2021

zalog commented Mar 26, 2021

feat: adds no wrap demo, refactors Y margins #22

feat: adds no wrap demo, refactors Y margins #22

Conversation

MickL commented Jan 24, 2021 • edited by zalog Loading

zalog commented Jan 31, 2021 • edited Loading

DKhalil commented Jan 31, 2021 • edited Loading

zalog Jan 31, 2021

Choose a reason for hiding this comment

zalog Jan 31, 2021 • edited Loading

Choose a reason for hiding this comment

zalog Jan 31, 2021

Choose a reason for hiding this comment

zalog commented Jan 31, 2021 • edited Loading

MickL commented Jan 31, 2021 • edited Loading

zalog commented Jan 31, 2021 • edited Loading

MickL commented Feb 1, 2021 • edited Loading

zalog commented Feb 3, 2021

MickL commented Mar 8, 2021

zalog commented Mar 8, 2021

MickL commented Mar 13, 2021 • edited Loading

MickL commented Mar 13, 2021

zalog commented Mar 14, 2021 • edited Loading

zalog commented Mar 16, 2021 • edited Loading

zalog commented Mar 21, 2021

zalog commented Mar 26, 2021

DKhalil commented Mar 26, 2021

MickL commented Mar 26, 2021

zalog commented Mar 26, 2021 • edited Loading

MickL commented Mar 26, 2021

zalog commented Mar 26, 2021

MickL commented Jan 24, 2021 •

edited by zalog

Loading

zalog commented Jan 31, 2021 •

edited

Loading

DKhalil commented Jan 31, 2021 •

edited

Loading

zalog Jan 31, 2021 •

edited

Loading

zalog commented Jan 31, 2021 •

edited

Loading

MickL commented Jan 31, 2021 •

edited

Loading

zalog commented Jan 31, 2021 •

edited

Loading

MickL commented Feb 1, 2021 •

edited

Loading

MickL commented Mar 13, 2021 •

edited

Loading

zalog commented Mar 14, 2021 •

edited

Loading

zalog commented Mar 16, 2021 •

edited

Loading

zalog commented Mar 26, 2021 •

edited

Loading