magfest-archive / guests

Magfest band management plugin for ubersystem
GNU Affero General Public License v3.0
1 stars 1 forks source link

Fixes #85: Changes checklist wording and sorts checklist by date #86

Closed RobRuana closed 7 years ago

RobRuana commented 7 years ago

Changes checklist wording and sorts checklist by date. Uses the new macros in a lot of places, to get the nice auto-linking of urls.

Also adds two columns:

Note: this depends on https://github.com/magfest/ubersystem/pull/2787

RobRuana commented 7 years ago

You certainly don't have to approve this. However, you'll be hard pressed to convince me that this:

{% if band.panel.name %}
<div class="row">
    <label class="col-sm-2 control-label">Panel Name:</label>
    <div class="col-sm-6">
        {{ band.panel.name }}
    </div>
</div>
{% endif %}

{% if band.panel.length %}
<div class="row">
    <label class="col-sm-2 control-label">Panel Length:</label>
    <div class="col-sm-6">
        {{ band.panel.length }}
    </div>
</div>
{% endif %}

{% if band.panel.desc %}
<div class="row">
    <label class="col-sm-2 control-label">Panel Description:</label>
    <div class="col-sm-6">
        {{ band.panel.desc|linebreaksbr }}
    </div>
</div>
{% endif %}

{% if band.panel.tech_needs %}
<div class="row">
    <label class="col-sm-2 control-label">Panel Tech Needs:</label>
    <div class="col-sm-6">
        {{ band.panel.panel_tech_needs_labels|join(" / ") }}
    </div>
</div>
{% endif %}

Is somehow superior to this:

{{ macros.form_group(band.panel, 'name', label='Panel Name', is_readonly=True) }}
{{ macros.form_group(band.panel, 'length', label='Panel Length', is_readonly=True) }}
{{ macros.form_group(band.panel, 'desc', label='Panel Description', is_readonly=True) }}
{% if band.panel.tech_needs or band.panel.other_tech_needs %}
  {{ macros.form_checkgroup(band.panel, 'tech_needs',
      other_field='other_tech_needs',
      label='Panel Tech Needs',
      is_readonly=True) }}
{% endif %}

Especially considering that we get additional functionality for free by using the macros – like auto linkify. That being said, I always welcome any improvements, especially when it comes to architecture.

Perhaps instead of saying "I don't like this", it would be more constructive to say "Here is an alternative that offers these specific benefits".

kitsuta commented 7 years ago

You're definitely right in that the new code is much shorter! :) But for me, implementing these macros represents a totally different direction with templating that I thought we'd be going in, and the fact that it's happening without any discussion or even (it feels) opportunity for discussion is mostly what I have reservations about. Because it's just a few days before pre-reg launch, it feels like there's no time to stop and say "Okay, let's examine this approach and make sure we're all on the same page about it."

I did have an alternate proposal -- I sent it to you in a Google doc months ago, and was told that we don't have time to do it. You probably don't feel as though using macros is antithetical to that, and they could probably work within that system, so I get why you wouldn't think much of implementing them. But... seriously, four days before pre-reg launch? :P Slow down!

That all being said, we can always undo stuff and it's not like there's something immediately wrong with macros. I would just like to, I guess, have a chance to think through the macros approach before we do much more converting of templates.

RobRuana commented 7 years ago

Erg, I can't find the doc you wrote on templates anywhere 😢

kitsuta commented 7 years ago

No worries, I'll resend it. :)