feat: adds no wrap demo, refactors Y margins

[MickL]

• 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]

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]

• 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]

"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]

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]

"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]

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]

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]

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]

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.

[MickL]

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]

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]

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

[zalog]

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]

Hi @MickL,

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

[zalog]

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

[DKhalil]

@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]

Cols AND breakpoints are important imo

[zalog]

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]

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]

Tnks @MickL

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