rpocklin / angular-timeline

An Angular.JS directive that generates a responsive, data-driven vertical timeline to tell a story, show history or describe a sequence of events.
Other
416 stars 108 forks source link

Namespace directives #7

Closed ThomasBurleson closed 9 years ago

ThomasBurleson commented 9 years ago

I recommend you namespace your directives and CSS:

angular.module('angular-timeline').directive('rp-timeline', function() {... }
angular.module('angular-timeline').directive('rp-timeline-panel', function() { ... }

Note the additional hypens in np-timeline-panel

.rp-timeline {
  list-style: none;
  padding: 20px 0 20px;
  position: relative; 
}
ThomasBurleson commented 9 years ago

Btw - Nice library.

ThomasBurleson commented 9 years ago

I also recommend you reduce the clutter of you directives in the HTML markup. Some directives, IMO, could be CSS styles. Here is an idea:

<rp-timeline-item side="left">

    <i class="info glyphicon glyphicon-check" rp-timeline-badge></i>

    <rp-timeline-panel>

        <!-- heading for the panel -->
        <rp-timeline-heading>
            <span rp-timeline-title>Some heading</span>
            <p>
                <small class="text-muted"><i class="glyphicon glyphicon-time"></i> 11 hours ago via Twitter</small>
            </p>
        </rp-timeline-heading>

        <!-- anything not in a `rp-timeline-heading` is content for the panel -->
        <div class="rp-timeline-content">
          <p>blah blah blah</p>
        </div>

    </rp-timeline-panel>

</rp-timeline-item>
rpocklin commented 9 years ago

Hi. I'll try and respond to all your comments in this reply

  1. Unless there's a specific library this is conflicting with I don't see it changing soon. It would be a version break and i'd do it for all my directives. I agree it's a good practice.
  2. I love the animations. I'd make it optional (ie. another theme or class to specify) but yes that's awesome.
  3. Everyone is going to have their own suggestions on this one. If you've got a strong opinion on the best way and why, happy to consider it. Many of the elements are optional, perhaps the docs should include a minimal example to illustrate this but I haven't had much time for this.
ThomasBurleson commented 9 years ago

The HTML example above is based on lessons learned as my team implemented Angular Material.

We found concise, clear markup without overdoing the API (eg. markup) works best.

For example, I could improve with a second iteration:

<timeline-item side="left">
    <!-- badge for the vertical timeline -->

    <timeline-badge>
        <div class="info glyphicon glyphicon-check" />
    </timeline-badge>

    <!-- heading for the panel -->

    <timeline-heading>
        <span timeline-title>Some heading</span>
        <p>
            <small class="text-muted"><i class="glyphicon glyphicon-time"></i> 11 hours ago via Twitter</small>
        </p>
    </timeline-heading>

    <!-- anything not in a `heading` or `badge` is content for the panel -->

    <div class="timeline-content">
        <p>blah blah blah</p>
    </div>

</timeline-item>
rpocklin commented 9 years ago

Thanks for all the feedback.

I've taken on your points about decreasing the custom elements. I've also added replace: true and incorporated another directive I released to give the 'bounce' animation on view (https://github.com/rpocklin/angular-scroll-animate).

Feel free to take a look at the new angular-timeline demo, any feedback is welcome. http://rp.js.org/angular-timeline/example/index.html

For the time being I haven't got plans to prefix the CSS and directive but it's a good point you make.

ThomasBurleson commented 9 years ago

@rpocklin - you can also use GSAP to easily stagger your animations. The new demo is superb.And the DOM looks much cleaner. Nice work.

rpocklin commented 9 years ago

Cheers - I think that closes this issue then :)