From 220b1de01e8b3126190996b889f7afbb6d5bb60b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 14 Nov 2020 14:08:36 +0100 Subject: [PATCH 1/4] Remove duplicate text in table of contents We were using different divs to show the same text in different positions, but we can use the same one and rotate it when appropriate. --- .../stylesheets/legislation_process.scss | 75 ++++++++----------- .../draft_versions/_comments_panel.html.erb | 7 +- .../legislation/draft_versions/show.html.erb | 8 +- 3 files changed, 33 insertions(+), 57 deletions(-) diff --git a/app/assets/stylesheets/legislation_process.scss b/app/assets/stylesheets/legislation_process.scss index 0d54f87ab..fd945788a 100644 --- a/app/assets/stylesheets/legislation_process.scss +++ b/app/assets/stylesheets/legislation_process.scss @@ -326,6 +326,33 @@ position: relative; padding: rem-calc(32) 0; + %calc-collapsed { + .draft-panel { + @include breakpoint(medium) { + line-height: 1; + display: block; + font-size: $small-font-size; + text-transform: uppercase; + font-weight: 700; + color: #696969; + padding: 0; + text-align: center; + + .icon-banner::before, + .icon-comments::before { + display: block; + margin: rem-calc(24) auto rem-calc(36); + } + + .panel-title { + display: block; + transform: rotate(-90deg); + filter: progid:DXImageTransform.Microsoft.BasicImage(rotation=3); + } + } + } + } + .draft-chooser { h3 { @@ -388,10 +415,6 @@ .draft-panel { cursor: pointer; } - - .draft-index-rotated { - display: none; - } } @media screen and (min-width: 40em) { @@ -445,7 +468,6 @@ line-height: 0; color: $text-medium; vertical-align: sub; - margin-right: rem-calc(4); } } @@ -536,58 +558,27 @@ } } - .calc-comments { + &:not(.comments-on) .calc-comments { + @extend %calc-collapsed; position: relative; .comment-box { display: none; } - - .draft-comments-rotated { - @include breakpoint(medium) { - font-size: $small-font-size; - text-transform: uppercase; - font-weight: 700; - color: #696969; - margin-top: rem-calc(64); - transform: rotate(-90deg); - filter: progid:DXImageTransform.Microsoft.BasicImage(rotation=3); - } - } } } .comments-on { .calc-index { + @extend %calc-collapsed; + width: rem-calc(50); background: #f2f2f2; cursor: pointer; - .panel-title { - display: none; - } - .draft-index { display: none; } - - .draft-index-rotated { - @include breakpoint(medium) { - line-height: 1; - display: block; - font-size: $small-font-size; - text-transform: uppercase; - font-weight: 700; - color: #696969; - margin-top: $line-height; - transform: rotate(-90deg); - filter: progid:DXImageTransform.Microsoft.BasicImage(rotation=3); - - .panel-title { - display: block; - } - } - } } .calc-text { @@ -617,10 +608,6 @@ cursor: pointer; } - .draft-comments-rotated { - display: none; - } - .comments-box-container { position: absolute; top: rem-calc(230); diff --git a/app/views/legislation/draft_versions/_comments_panel.html.erb b/app/views/legislation/draft_versions/_comments_panel.html.erb index 41d26c80f..853ae6f49 100644 --- a/app/views/legislation/draft_versions/_comments_panel.html.erb +++ b/app/views/legislation/draft_versions/_comments_panel.html.erb @@ -1,11 +1,6 @@
-
- <%= t("legislation.draft_versions.show.text_comments") %> -
-
- -
+ <%= t("legislation.draft_versions.show.text_comments") %>
diff --git a/app/views/legislation/draft_versions/show.html.erb b/app/views/legislation/draft_versions/show.html.erb index 2123fa61b..90e3da79f 100644 --- a/app/views/legislation/draft_versions/show.html.erb +++ b/app/views/legislation/draft_versions/show.html.erb @@ -45,13 +45,7 @@
">
-
- - <%= t("legislation.draft_versions.show.text_toc") %> -
-
- -
+ <%= t("legislation.draft_versions.show.text_toc") %>
From 41e5ddbcdfcc99db0f3ca3dc9c95b06ea8bdee7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 14 Nov 2020 12:50:30 +0100 Subject: [PATCH 2/4] Use details tag to show/hide a draft version TOC We were using JavaScript to show/hide the Table of Contents. In my humble opinion, the
tag has a few shortcomings [1][2], which means we should be careful about when to use it. IMHO a Table of Contents is a good candidate for this tag because it's a very common pattern to add a show/hide behavior for it, even if using it means the "navigation" role (which we are *not* using anyway) wouldn't be identified correctly. I'm adding a
tag to the comments section as well for consistency and in order to simplify the code. I'm not sure this is as good an application of the
tag, though, but then again I'm not sure about the interface we use to show/hide the comments (and this feeling is increased by the fact that we use a different interface on small screens). If we decide to change the interface in the future, we might consider using the
tag for the Table of Contents but not for the comments. Since the
tag is not supported on Internet Explorer, I'm only adding styles to this tag using the `:not([open])` option. On Internet Explorer
will always be opened and so these styles will be ignored. [1] https://adrianroselli.com/2019/04/details-summary-are-not-insert-control-here.html [2] https://daverupert.com/2019/12/why-details-is-not-an-accordion/ --- app/assets/javascripts/application.js | 1 - .../javascripts/legislation_allegations.js | 28 +---- .../stylesheets/legislation_process.scss | 114 +++++++----------- .../draft_versions/_comments_panel.html.erb | 8 +- .../legislation/draft_versions/show.html.erb | 16 ++- 5 files changed, 57 insertions(+), 110 deletions(-) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 49bbf8b2b..29ee399ef 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -139,7 +139,6 @@ var initialize_modules = function() { App.MarkdownEditor.initialize(); App.HTMLEditor.initialize(); App.LegislationAdmin.initialize(); - App.LegislationAllegations.initialize(); App.Legislation.initialize(); if ($(".legislation-annotatable").length) { App.LegislationAnnotatable.initialize(); diff --git a/app/assets/javascripts/legislation_allegations.js b/app/assets/javascripts/legislation_allegations.js index f5970b8e5..fe1f596be 100644 --- a/app/assets/javascripts/legislation_allegations.js +++ b/app/assets/javascripts/legislation_allegations.js @@ -1,36 +1,10 @@ (function() { "use strict"; App.LegislationAllegations = { - toggle_comments: function() { - if (!App.LegislationAnnotatable.isMobile()) { - $(".draft-allegation").toggleClass("comments-on"); - $("#comments-box").html("").hide(); - } - }, show_comments: function() { if (!App.LegislationAnnotatable.isMobile()) { - $(".draft-allegation").addClass("comments-on"); + document.querySelector(".draft-allegation details.calc-comments").open = true; } - }, - initialize: function() { - $(".js-toggle-allegations .draft-panel").on({ - click: function(e) { - e.preventDefault(); - e.stopPropagation(); - if (!App.LegislationAnnotatable.isMobile()) { - App.LegislationAllegations.toggle_comments(); - } - } - }); - $(".js-toggle-allegations").on({ - click: function() { - if (!App.LegislationAnnotatable.isMobile()) { - if ($(this).find(".draft-panel .panel-title:visible").length === 0) { - App.LegislationAllegations.toggle_comments(); - } - } - } - }); } }; }).call(this); diff --git a/app/assets/stylesheets/legislation_process.scss b/app/assets/stylesheets/legislation_process.scss index fd945788a..74f034606 100644 --- a/app/assets/stylesheets/legislation_process.scss +++ b/app/assets/stylesheets/legislation_process.scss @@ -326,33 +326,6 @@ position: relative; padding: rem-calc(32) 0; - %calc-collapsed { - .draft-panel { - @include breakpoint(medium) { - line-height: 1; - display: block; - font-size: $small-font-size; - text-transform: uppercase; - font-weight: 700; - color: #696969; - padding: 0; - text-align: center; - - .icon-banner::before, - .icon-comments::before { - display: block; - margin: rem-calc(24) auto rem-calc(36); - } - - .panel-title { - display: block; - transform: rotate(-90deg); - filter: progid:DXImageTransform.Microsoft.BasicImage(rotation=3); - } - } - } - } - .draft-chooser { h3 { @@ -411,12 +384,6 @@ padding-right: rem-calc(15); } - .calc-index { - .draft-panel { - cursor: pointer; - } - } - @media screen and (min-width: 40em) { .calc-index { width: calc(35% - 25px); @@ -425,19 +392,6 @@ .calc-text { width: calc(65% - 25px); } - - .calc-comments { - cursor: pointer; - background: #f2f2f2; - width: rem-calc(50); - - .draft-panel { - - .panel-title { - display: none; - } - } - } } .border-right { @@ -558,26 +512,51 @@ } } - &:not(.comments-on) .calc-comments { - @extend %calc-collapsed; + .calc-comments { position: relative; + } - .comment-box { + summary { + cursor: pointer; + list-style: none; + + &::-webkit-details-marker { display: none; } } - } - .comments-on { - .calc-index { - @extend %calc-collapsed; - - width: rem-calc(50); + details:not([open]) { background: #f2f2f2; - cursor: pointer; - .draft-index { - display: none; + @include breakpoint(small only) { + border-bottom: 1px solid #d4d4d4; + } + + @include breakpoint(medium) { + width: rem-calc(50); + + .draft-panel { + line-height: 1; + display: block; + font-size: $small-font-size; + text-transform: uppercase; + font-weight: 700; + color: #696969; + padding: 0; + text-align: center; + + .icon-banner::before, + .icon-comments::before { + display: block; + margin: rem-calc(24) auto rem-calc(36); + } + + .panel-title { + display: block; + transform: rotate(-90deg); + filter: progid:DXImageTransform.Microsoft.BasicImage(rotation=3); + } + } } } @@ -604,8 +583,14 @@ cursor: auto; width: calc(35% - 25px); - .draft-panel { - cursor: pointer; + @include breakpoint(small only) { + summary { + display: none; + } + } + + &:not([open]) { + border-left: 1px solid #d4d4d4; } .comments-box-container { @@ -790,15 +775,6 @@ } } } - - .draft-panel { - background: #e5e5e5; - border-left: 1px solid #d4d4d4; - - .panel-title { - display: inline-block; - } - } } } } diff --git a/app/views/legislation/draft_versions/_comments_panel.html.erb b/app/views/legislation/draft_versions/_comments_panel.html.erb index 853ae6f49..c9367f7e8 100644 --- a/app/views/legislation/draft_versions/_comments_panel.html.erb +++ b/app/views/legislation/draft_versions/_comments_panel.html.erb @@ -1,8 +1,8 @@ -
-
+
+ <%= t("legislation.draft_versions.show.text_comments") %> -
+
-
+ diff --git a/app/views/legislation/draft_versions/show.html.erb b/app/views/legislation/draft_versions/show.html.erb index 90e3da79f..428a07d7c 100644 --- a/app/views/legislation/draft_versions/show.html.erb +++ b/app/views/legislation/draft_versions/show.html.erb @@ -42,19 +42,17 @@ <%= render "legislation/processes/help_gif" %> -
-
"> -
+
+
+ <%= t("legislation.draft_versions.show.text_toc") %> -
+ -
-
- <%= AdminLegislationSanitizer.new.sanitize(@draft_version.toc_html) %> -
+
+ <%= AdminLegislationSanitizer.new.sanitize(@draft_version.toc_html) %>
-
+
<%= t("legislation.draft_versions.show.text_body") %>
From 48daf22f31682ff845f759ebdcca537e0f04a92d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 14 Nov 2020 19:26:59 +0100 Subject: [PATCH 3/4] Make draft version content use the empty space Now that comments and TOC can be closed at the same time, we use a flex layout so the main content uses the available width. We're also making the comments work better on medium-sized screens, since previously they had a fixed width and now the width is adapted to the size of the screen. Since now the comment box element has a relative position instead of an absolute one, we need to consider the draft panel height when calculating the comment box position. --- .../javascripts/legislation_annotatable.js | 4 ++- .../stylesheets/legislation_process.scss | 31 ++++++++++++------- .../draft_versions/_comments_panel.html.erb | 2 +- .../legislation/draft_versions/show.html.erb | 8 ++--- 4 files changed, 28 insertions(+), 17 deletions(-) diff --git a/app/assets/javascripts/legislation_annotatable.js b/app/assets/javascripts/legislation_annotatable.js index 3959dc5eb..85743b305 100644 --- a/app/assets/javascripts/legislation_annotatable.js +++ b/app/assets/javascripts/legislation_annotatable.js @@ -40,8 +40,10 @@ }, renderAnnotationComments: function(event) { if (event.offset) { + var default_top = $(".calc-comments").offset().top + $(".calc-comments .draft-panel").outerHeight(); + $("#comments-box").css({ - top: event.offset - $(".calc-comments").offset().top + top: event.offset - default_top }); } if (App.LegislationAnnotatable.isMobile()) { diff --git a/app/assets/stylesheets/legislation_process.scss b/app/assets/stylesheets/legislation_process.scss index 74f034606..a10744841 100644 --- a/app/assets/stylesheets/legislation_process.scss +++ b/app/assets/stylesheets/legislation_process.scss @@ -380,17 +380,26 @@ @include breakpoint(medium) { display: flex; - padding-left: rem-calc(15); - padding-right: rem-calc(15); - } - @media screen and (min-width: 40em) { .calc-index { - width: calc(35% - 25px); + max-width: calc(35% - 25px); + + > * { + padding-right: rem-calc(20); + } } .calc-text { - width: calc(65% - 25px); + flex: 1; + } + + .calc-comments { + min-width: 15rem; + width: calc(35% - 25px); + + > * { + padding-left: rem-calc(20); + } } } @@ -467,6 +476,8 @@ padding: rem-calc(16); @include breakpoint(medium) { + margin: 0 auto; + max-width: 3 * $grid-row-width / 4; padding: rem-calc(32) rem-calc(32) rem-calc(32) rem-calc(48); } @@ -533,6 +544,7 @@ } @include breakpoint(medium) { + min-width: auto; width: rem-calc(50); .draft-panel { @@ -561,7 +573,6 @@ } .calc-text { - width: calc(65% - 25px); border-right: 0; .show-comments { @@ -581,7 +592,6 @@ .calc-comments { background: #fff; cursor: auto; - width: calc(35% - 25px); @include breakpoint(small only) { summary { @@ -594,12 +604,11 @@ } .comments-box-container { - position: absolute; - top: rem-calc(230); + position: relative; + top: rem-calc(180); } .comment-box { - width: rem-calc(375); padding: rem-calc(4); background: #f9f9f9; border: 1px solid $border; diff --git a/app/views/legislation/draft_versions/_comments_panel.html.erb b/app/views/legislation/draft_versions/_comments_panel.html.erb index c9367f7e8..178fca4e3 100644 --- a/app/views/legislation/draft_versions/_comments_panel.html.erb +++ b/app/views/legislation/draft_versions/_comments_panel.html.erb @@ -1,4 +1,4 @@ -
+
<%= t("legislation.draft_versions.show.text_comments") %> diff --git a/app/views/legislation/draft_versions/show.html.erb b/app/views/legislation/draft_versions/show.html.erb index 428a07d7c..f4f1ad0ed 100644 --- a/app/views/legislation/draft_versions/show.html.erb +++ b/app/views/legislation/draft_versions/show.html.erb @@ -42,8 +42,8 @@ <%= render "legislation/processes/help_gif" %> -
-
+
+
<%= t("legislation.draft_versions.show.text_toc") %> @@ -53,7 +53,7 @@ <%= AdminLegislationSanitizer.new.sanitize(@draft_version.toc_html) %>
-
+
<%= t("legislation.draft_versions.show.text_body") %>
@@ -72,7 +72,7 @@
<% if @draft_version.final_version? %> -
+
<% else %> <%= render "comments_panel", draft_version: @draft_version %> <% end %> From 0961cf464ca95f6e26a980afb40816b3b67a99a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 14 Nov 2020 20:47:36 +0100 Subject: [PATCH 4/4] Use CSS to make Table of Contents sticky Originally we were using Foundation's sticky, which wasn't entirely compatible with our way to open/close the Table of Contents because its width would not automatically be updated when the TOC was opened/closed but when users scrolled the page. Using CSS, which is now supported in most browsers, simplifies the matter. On browsers like Internet Explorer, where it's not supported, the content will not stick but other than that it'll work fine. We're also adding `scroll: auto` so when the TOC's height will be large than the page, it'll be possible to scroll it, which users couldn't do in the original version. --- app/assets/stylesheets/legislation_process.scss | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/assets/stylesheets/legislation_process.scss b/app/assets/stylesheets/legislation_process.scss index a10744841..0b557a869 100644 --- a/app/assets/stylesheets/legislation_process.scss +++ b/app/assets/stylesheets/legislation_process.scss @@ -384,6 +384,15 @@ .calc-index { max-width: calc(35% - 25px); + .draft-index { + @supports (position: sticky) { + max-height: 100vh; + overflow-y: auto; + position: sticky; + top: $line-height; + } + } + > * { padding-right: rem-calc(20); }