fix: More fixes for narrow screens by larseggert · Pull Request #3662 · ietf-tools/datatracker

larseggert · 2022-03-17T15:44:48Z

Need to keep checking for issues, there are more.

rjsparks

Please verify the new loop construction in requests.html doesn't leave an unclosed <tbody>

rjsparks · 2022-03-18T23:25:28Z

                                                    {% else %}
                                                        {% with constraint.name.slug as constraint_name_slug %}
-                                                            <span class="{% if constraint_name_slug == 'conflict' %}text-danger{% elif constraint_name_slug == 'conflic2' %}text-warning{% elif constraint_name_slug == 'conflic3' %}text-success{% else %}{{ constraint_name_slug }}{% endif %}">
+                                                            <span class="{% if constraint_name_slug == 'chair_conflict' %}text-danger{% elif constraint_name_slug == 'tech_overlap' %}text-info{% elif constraint_name_slug == 'key_participant' %}text-success{% else %}{{ constraint_name_slug }}{% endif %}">


This should become aware of the constraints in use by a given meeting: https://github.com/ietf-tools/datatracker/blob/main/ietf/meeting/models.py#L125.

Might be a spot for a template filter to convert a constraint into a "badness" level

I can do a filter, but the line Robert indicated doesn't give me enough information about what the desired behavior is?

this is probably bigger than this first pass and should be split into a separate ticket. A meeting knows which constraint names are in use. Right now, we have hardcoded how to present each constraint into the template (chair_conflict is text-danger, etc). We would need to add a filter, as jennifer is suggesting, that encapsulated what class to provide for each constraint type.

larseggert · 2022-03-19T05:43:54Z

I don't think it does - the tidy check that the test suite now runs would have caught that.

rjsparks · 2022-03-19T09:52:11Z

The tidy check must not do what you think it does.

I checked out the PR and looked at the html generated by ietf.meeting.tests_views.SessionTests.test_meeting_requests.
I put it in a file and got this result:

rjsparks@undex lars % grep tbody ~/tmp/foo.html
                    <tbody>
                    </tbody>
                    <tbody>

Looking into the call to tidy:

    r.status_code: '200'
    r["content-type"]: 'text/html; charset=utf-8'
    skip_verify: 'False'
    errors: 'line 430 column 13 - Warning: trimming empty <i>
line 527 column 21 - Warning: trimming empty <i>
line 520 column 17 - Warning: trimming empty <button>
line 598 column 17 - Warning: trimming empty <i>
line 638 column 17 - Warning: trimming empty <i>
line 687 column 17 - Warning: trimming empty <i>
line 730 column 29 - Warning: trimming empty <i>
line 735 column 33 - Warning: trimming empty <i>
'

larseggert · 2022-03-19T13:25:01Z

I looked into this, and HTML Tidy misses a bunch of HTML errors. Gah. I am adding HTML5 validation based on vnu in #3682 and will rework this PR once that is done.

jennifer-richards

Looks ok to me aside from what @rjsparks already flagged

jennifer-richards · 2022-03-19T12:48:35Z

            Action Holder{{ doc.documentactionholder_set.all|pluralize }}:
            {% for action_holder in doc.documentactionholder_set.all %}
-                {% person_link action_holder.person title=action_holder.role_for_doc %} {{ action_holder|action_holder_badge }}{% if not forloop.last %},{% endif %}
+                {% person_link action_holder.person title=action_holder.role_for_doc %}{% if action_holder|action_holder_badge %} {{ action_holder|action_holder_badge }}{% endif %}{% if not forloop.last %},{% endif %}


I think the {% if... %} wrapper should be a no-op. The action_holder-badge tag returns '' if there's nothing to include, so either way there should be no text inserted. Is this doing something subtle?

The issue is that if there is no action_holder, there should also not be a space before the (empty) badge. (And if there is one, there should be a space.) Is there a better way to do this?

Look to see if that space is appropriate for everywhere the action_holder templatetag is used. If it is, then add it to the html that the templatetag emits when a badge should be present.

Or, to make it much more obvious why you're adding the space, try

{% if action_holder|action_holder_badge %} {% endif %}{% action_holder|action_holder_badge %}

Or better yet, look to see if action_holder_badge being empty only happens when there is no action holder, and if so reduce that to

{% if action_holder %} {% endif %}{% action_holder|action_holder_badge %}

But this isn't enough to hold up the PR over.

jennifer-richards · 2022-03-19T13:51:52Z

                                                    {% else %}
                                                        {% with constraint.name.slug as constraint_name_slug %}
-                                                            <span class="{% if constraint_name_slug == 'conflict' %}text-danger{% elif constraint_name_slug == 'conflic2' %}text-warning{% elif constraint_name_slug == 'conflic3' %}text-success{% else %}{{ constraint_name_slug }}{% endif %}">
+                                                            <span class="{% if constraint_name_slug == 'chair_conflict' %}text-danger{% elif constraint_name_slug == 'tech_overlap' %}text-info{% elif constraint_name_slug == 'key_participant' %}text-success{% else %}{{ constraint_name_slug }}{% endif %}">


Might be a spot for a template filter to convert a constraint into a "badness" level

jennifer-richards · 2022-03-19T14:30:49Z

Oops, didn't realize my approval would authorize github to allow merge - should have just been a comment but I don't see a way to change that.

Need to keep checking for issues, there are more.

larseggert · 2022-03-22T10:00:21Z

Modulo the open question about the filter, this PR is ready from my side.

larseggert marked this pull request as ready for review March 18, 2022 11:00

rjsparks requested changes Mar 18, 2022

View reviewed changes

rjsparks requested a review from jennifer-richards March 18, 2022 23:29

jennifer-richards approved these changes Mar 19, 2022

View reviewed changes

jennifer-richards self-requested a review March 19, 2022 14:29

jennifer-richards removed their request for review March 19, 2022 14:31

larseggert mentioned this pull request Mar 21, 2022

ci: Also check generated HTML with the "vnu" validator #3682

Merged

larseggert added 6 commits March 22, 2022 09:08

fix: More fixes for narrow screens

aee4e86

Need to keep checking for issues, there are more.

Make columns more narrow on narrow screens

8f69c4a

More fixes

fcc8825

More fixes

6e2e2ca

Test fixes

c91d471

Fix more tests

54cba46

larseggert force-pushed the more-fixes-for-narrow-screens branch from 507e200 to 54cba46 Compare March 22, 2022 08:52

Close tbody

366b281

larseggert mentioned this pull request Mar 24, 2022

Restore indicator of and link to current row on agenda painless-security/datatracker#3

Merged

Merge branch 'feat/bs5' into more-fixes-for-narrow-screens

dab3fdd

jennifer-richards merged commit 870fed0 into ietf-tools:feat/bs5 Mar 24, 2022

This was referenced Mar 24, 2022

Improve whitespace handling around action_holder_badge template tag #3744

Closed

Display session constraints properly for configurable constraint types #3745

Open

larseggert deleted the more-fixes-for-narrow-screens branch April 26, 2022 18:03

github-actions Bot locked as resolved and limited conversation to collaborators Jun 16, 2022

Conversation

larseggert commented Mar 17, 2022

Uh oh!

rjsparks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rjsparks Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

jennifer-richards Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

larseggert Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

rjsparks Mar 22, 2022

Choose a reason for hiding this comment

Uh oh!

larseggert commented Mar 19, 2022

Uh oh!

rjsparks commented Mar 19, 2022

Uh oh!

larseggert commented Mar 19, 2022

Uh oh!

jennifer-richards left a comment

Choose a reason for hiding this comment

Uh oh!

jennifer-richards Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

larseggert Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjsparks Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jennifer-richards Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

jennifer-richards commented Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larseggert commented Mar 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

larseggert Mar 22, 2022 •

edited

Loading

rjsparks Mar 22, 2022 •

edited

Loading

jennifer-richards commented Mar 19, 2022 •

edited

Loading