R9 id card semester select #78
Conversation
index.yaml
Outdated
| @@ -10,8 +10,46 @@ indexes: | |||
| # automatically uploaded to the admin console when you next deploy | |||
| # your application using appcfg.py. | |||
|
|
|||
| - kind: BridgeCrew | |||
There was a problem hiding this comment.
We should probably update the version of index.yaml in the repo or .gitignore it, but I do not think this is the place.
There was a problem hiding this comment.
Rebasing of master (where index.yaml was removed from the repo and added to the .gitignore) should hopefully resolve this.
| 'members': members, | ||
| 'semester': semester_mod | ||
| # Replace 0 with O because it looks better in the font. | ||
| 'semester': semester_pretty(semester).replace('0', 'O') |
There was a problem hiding this comment.
After IRL discussion at today's meeting, disregard.
semesters.py
Outdated
| # Don't accept anything that does not cast to a float | ||
| except ValueError: return None | ||
|
|
||
| # Reject semesters that are not in this milennia |
semesters.py
Outdated
| if semester > 3000 or semester < 2000: | ||
| return None | ||
| # Reject semesters that are not Fall or Spring | ||
| elif round(semester - int(semester), 1) not in [.1, .2]: |
There was a problem hiding this comment.
I think the convention for this project is 0.1 and 0.2; correct me if I am wrong.
| template_vals = { | ||
| 'title': 'ID card for ' + member.name, | ||
| 'members': [member], | ||
| 'semester': semester_mod | ||
| # Replace 0 with O because it looks better in the font | ||
| 'semester': semester_pretty(semester).replace('0', 'O') |
There was a problem hiding this comment.
While this is less lines, the semester_mod (you are welcome to rename the variable if you so desire) line was intentionally separated out to be less confusing to future maintainers of the code (and it avoids having a comment in the middle of a dictionary literal).
There was a problem hiding this comment.
After IRL discussion at today's meeting, disregard.
|
@derekw023: Do you think you could have an updated version of this by tonight's meeting? |
|
Not for tonight, I have no time. We should probably discuss these and other requested changes at the meeting |
|
Are there any other changes required for this pull request? I don't remember if anything else came up at the meeting |
templates/admin.html
Outdated
| <h3>Select semester for which to generate ID cards:</h3> | ||
| <select name="semester" size="{{ semesters|length }}"> | ||
| {% for semester in semesters %} | ||
| <option value={{ semester }}{% if semester == current_semester%} selected="selected"{%endif%}>{{ semester }}</option> |
There was a problem hiding this comment.
Can you make the spacing here consistent (i.e. {% if ... current_semester%} → {% if ... current_semester %}; {%endif%} → {% endif %})?
templates/admin.html
Outdated
| <h2>Print All ID Cards</h2> | ||
| <form action=/cards/all> | ||
| <h3>Select semester for which to generate ID cards:</h3> | ||
| <select name="semester" size="{{ semesters|length }}"> |
There was a problem hiding this comment.
I am inclined to say make this a drop-down menu (i.e. no size) since the number of semesters will continue to increase, but people will probably only want to print one semester's ID cards.
templates/admin.html
Outdated
| </select> | ||
| <br /> | ||
| <br /> | ||
| <button type="submit">Print</button> |
There was a problem hiding this comment.
Can you add the printer emoji, as used on the member list page, for consistency? (I think it is 🖶.)
dexw25
force-pushed
the
r8-id-card-semester-select
branch
from
5955450
to
e4e2922
Compare
…s page to select based on specified semester
|
Requested changes addressed and rebased to master (index.yaml removed) |
|
Good job :P
…On Wed, May 9, 2018 at 11:18 PM, Derek Witt ***@***.***> wrote:
Requested changes addressed and rebased to master (index.yaml removed)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#78 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/Adrsc5iVAI_9oBUqibxny0GvPgId6PKZks5tw7GbgaJpZM4Rqt46>
.
|
Resolves #65
Branch name states R8 but this was planned to be in R9