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] 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") %>