-
Notifications
You must be signed in to change notification settings - Fork 19
Add information about the journal above the fold in editorial form #2525
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 13 commits
6ed695b
97521a0
bd2fb5a
8799d76
8c51ae1
4d6a6ea
f261d05
80fa5d1
cf34f2f
c1c24a6
168549a
66487e9
afcd0bf
4199746
720db8c
b15dc58
4d4e364
d7c512f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,18 +1,76 @@ | ||
| /* Page header */ | ||
|
|
||
| .page-header { | ||
| min-height: 150px; | ||
| min-height: 150px; | ||
|
|
||
| @media screen and (max-width: 768px) { | ||
| min-height: 100px; | ||
| } | ||
| @media screen and (max-width: 768px) { | ||
| min-height: 100px; | ||
| } | ||
|
|
||
| /* do not underline the links, unless in an alert */ | ||
| a:not(.alert a) { | ||
| text-decoration: none; | ||
| } | ||
| /* do not underline the links, unless in an alert */ | ||
| a:not(.alert a) { | ||
| text-decoration: none; | ||
| } | ||
|
|
||
| .feather { | ||
| position: relative; | ||
| } | ||
| .feather { | ||
| position: relative; | ||
| } | ||
| } | ||
|
|
||
| .above_the_fold--title { | ||
| margin-bottom: 1rem; | ||
| } | ||
|
|
||
| .above_the_fold h2 { | ||
| margin-top: 0; | ||
| margin-bottom: 1rem; | ||
| } | ||
|
|
||
| .above_the_fold--url { | ||
| color: #5c5956; | ||
| } | ||
|
|
||
| .above_the_fold--issns { | ||
| font-size: 0.9rem; | ||
| font-weight: bold; | ||
| color: $sanguine; | ||
| margin-top: 0; | ||
| margin-bottom: 1rem !important; | ||
| } | ||
|
|
||
| .button.button--tertiary.see_in_doaj { | ||
| margin-bottom: 0; | ||
| } | ||
|
|
||
| @media (max-width: 1279px) and (min-width: 768px) { | ||
| .button.button--tertiary.see_in_doaj { | ||
| margin-top: 1rem; | ||
| } | ||
| } | ||
|
|
||
| @media (min-height: calc((10rem + 105px) * 2.5)) { | ||
| main:has(> .main-header.fixed) { | ||
| margin-top: 10rem; | ||
| @media (max-width: 1279px) and (min-width: 768px) { | ||
| &:has(.button.button--tertiary.see_in_doaj){ | ||
| margin-top: 13rem; | ||
| } | ||
| .button.button--tertiary.see_in_doaj { | ||
| margin-top: 1rem; | ||
| } | ||
| } | ||
| .button.button--tertiary.see_in_doaj { | ||
| margin-top: 1rem; | ||
| } | ||
|
|
||
| .main-header.fixed { | ||
| position: fixed; | ||
| top: 75px; | ||
| left: 75px; | ||
| z-index: 1000; | ||
| background-color: white; | ||
| width: calc(100% - 75px); | ||
| padding: 25px 30px; | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,10 +3,6 @@ | |
| .dashboard { | ||
| font-size: 18px; | ||
|
|
||
| h1 { | ||
| margin-top: 0; | ||
| } | ||
|
|
||
| > header:first-of-type { | ||
| height: 75px; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| suite: New Header in Editorial Forms | ||
| testset: New Header in Editorial Forms | ||
|
|
||
| tests: | ||
| - title: Admin Review Form Layout | ||
| setup: | ||
| - Go to /testdrive/new_editorial_form_layout | ||
| context: | ||
| role: admin | ||
| steps: | ||
| - step: Log in as BaronVonBurp, the admin | ||
| - step: Access the journal form at /admin/<journal_id> | ||
| results: | ||
| - The top section displays the journal title, URL, and ISSN(s) | ||
| - Each item has a copy button next to it | ||
| - Click each copy button and paste the value somewhere (e.g., Notepad) to confirm the correct value is copied | ||
| - The top section stays visible while scrolling, unless the screen is very short and the header takes more than 40% of the viewport | ||
| - step: Access the application form at /application/<application_id> | ||
| results: | ||
| - Same checks as above | ||
| - step: Access the update request form at /application/<update_request_id> | ||
| results: | ||
| - Same checks as above | ||
| - step: Click "Unlock & Close" on the update request page | ||
| - step: Log out of the admin account | ||
|
|
||
| - title: Editor Review Form Layout | ||
| setup: | ||
| - Go to /testdrive/new_editorial_form_layout | ||
| context: | ||
| role: editor | ||
| steps: | ||
| - step: Log in using as LadyGigglesworth, the editor | ||
| - step: Access the application form at /editor/application/<application_id> | ||
| results: | ||
| - The top section displays the journal title, URL, and ISSN(s) | ||
| - Each item has a copy button next to it | ||
| - Click each copy button and paste the value somewhere to confirm correctness | ||
| - The top section remains visible while scrolling (unless the screen is very short) | ||
| - step: Click "Unlock & Close" on the update request page | ||
| - step: Log out of the editor account | ||
|
|
||
| - title: Publisher Form Layout | ||
| setup: | ||
| - Go to /testdrive/new_editorial_form_layout | ||
| context: | ||
| role: publisher | ||
| steps: | ||
| - step: Log in using as SirSneezesALot, the publisher | ||
| - step: Access the update request form at /publisher/update_request/<journal_id> | ||
| results: | ||
| - The top section displays only the journal title | ||
| - There is no fixed/sticky section | ||
| - The form has not changed from the currently published version | ||
| - step: Click "Unlock & Close" | ||
| - step: Log out of the publisher account |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| from portality import constants | ||
| from doajtest.testdrive.factory import TestDrive | ||
| from doajtest.fixtures.v2.journals import JournalFixtureFactory | ||
| from doajtest.fixtures.v2.applications import ApplicationFixtureFactory | ||
| from portality import models | ||
|
|
||
|
|
||
| class NewEditorialFormLayout(TestDrive): | ||
| def setup(self) -> dict: | ||
| publisher_un = "SirSneezesALot" | ||
| publisher_pw = self.create_random_str() | ||
| publisher_acc = models.Account.make_account(publisher_un + "@example.com", publisher_un, "Publisher " + publisher_un, [constants.ROLE_PUBLISHER]) | ||
| publisher_acc.set_password(publisher_pw) | ||
| publisher_acc.save() | ||
|
|
||
| admin_un = "BaronVonBurp" | ||
| admin_pw = self.create_random_str() | ||
| admin_acc = models.Account.make_account(admin_un + "@example.com", admin_un, | ||
| "Admin " + admin_un, | ||
| [constants.ROLE_ADMIN]) | ||
| admin_acc.set_password(admin_pw) | ||
| admin_acc.save() | ||
|
|
||
| editor_un = "LadyGigglesworth" | ||
| editor_pw = self.create_random_str() | ||
| editor_acc = models.Account.make_account(editor_un + "@example.com", editor_un, | ||
| "Editor " + editor_un, | ||
| [constants.ROLE_EDITOR]) | ||
| editor_acc.set_password(editor_pw) | ||
| editor_acc.save() | ||
|
|
||
| source = JournalFixtureFactory.make_journal_source(in_doaj=True) | ||
| j = models.Journal(**source) | ||
| j.remove_current_application() | ||
| j.set_id(j.makeid()) | ||
| j.set_owner(publisher_acc.id) | ||
| j.bibjson().eissn = "1987-0007" | ||
| j.bibjson().pissn = "3141-5926" | ||
| j.bibjson().title = "Annals of Interspecies Etiquette" | ||
| j.bibjson().journal_url = "https://mind-your-penguin-manners.com" | ||
| j.save(blocking=True) | ||
|
|
||
| source = ApplicationFixtureFactory.make_application_source() | ||
| a = models.Application(**source) | ||
| a.remove_current_journal() | ||
| a.remove_related_journal() | ||
| a.application_type = constants.APPLICATION_TYPE_NEW_APPLICATION | ||
| a.set_id(a.makeid()) | ||
| a.set_editor(editor_acc.id) | ||
| a.set_owner(publisher_acc.id) | ||
| a.bibjson().eissn = "2718-2818" | ||
| a.bibjson().title = "Journal of Hypothetical Entomology" | ||
| a.bibjson().journal_url = "https://does-it-bite.com" | ||
| a.set_application_status(constants.APPLICATION_STATUS_IN_PROGRESS) | ||
| a.save(blocking=True) | ||
|
|
||
| source = ApplicationFixtureFactory.make_update_request_source() | ||
| ur = models.Application(**source) | ||
| ur.set_id(ur.makeid()) | ||
| ur.set_editor(editor_acc.id) | ||
| ur.set_owner(publisher_acc.id) | ||
| ur.bibjson().pissn = "0042-2718" | ||
| ur.bibjson().title = "Annals of Temporal Mechanics" | ||
| ur.bibjson().journal_url = "https://time-loops-and-tea.com" | ||
| ur.save(blocking=True) | ||
|
|
||
| return { | ||
| "accounts": { | ||
| "admin": {"username": admin_acc.id, | ||
| "password": admin_pw}, | ||
| "editor": {"username": editor_acc.id, | ||
| "password": editor_pw}, | ||
| "publisher": {"username": publisher_acc.id, | ||
| "password": publisher_pw}, | ||
| }, | ||
| "journals": {j.bibjson().title: j.id}, | ||
| "applications": {a.bibjson().title: a.id}, | ||
| "update_requests": {ur.bibjson().title :ur.id} | ||
| } | ||
|
|
||
| def teardown(self, params) -> dict: | ||
| for accid in ["admin", "editor", "publisher"]: | ||
| models.Account.remove_by_id(params["accounts"][accid]["username"]) | ||
| for title, id in params["journals"].items(): | ||
| models.Journal.remove_by_id(id) | ||
| for title, id in params["applications"].items(): | ||
| models.Application.remove_by_id(id) | ||
| for title, id in params["update_requests"].items(): | ||
| models.Application.remove_by_id(id) | ||
| return {"status": "success"} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,8 @@ | |
| <!-- Glyphicons --> | ||
| <link rel="stylesheet" href="/static/doaj/css/glyphicons.css?v={{config.get('DOAJ_VERSION')}}"> | ||
|
|
||
| {% include 'includes/_js_includes.html' %} | ||
|
||
|
|
||
| </head> | ||
|
|
||
| <body class="{% block body_class %}{% endblock %}" | ||
|
|
@@ -60,8 +62,6 @@ | |
|
|
||
| {% block base_content %}{% endblock %} | ||
|
|
||
| {% include 'includes/_js_includes.html' %} | ||
|
|
||
| {% if not request.cookies.get("doaj-consent") %} | ||
| <script type="text/javascript"> | ||
| jQuery(document).ready(function() { | ||
|
|
||
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| <style> | ||
| .click-to-copy { | ||
| cursor: pointer; | ||
| margin: 0; | ||
| padding: 0; | ||
| width: 1.5em; | ||
| height: 1.5em; | ||
| display: inline-flex; | ||
| justify-content: center; | ||
| border: none; | ||
| } | ||
| .click-to-copy:hover, | ||
| .click-to-copy:focus, | ||
| .click-to-copy:active { | ||
| background-color: transparent; | ||
| } | ||
|
|
||
| .click-to-copy:hover .feather, | ||
| .click-to-copy:focus .feather, | ||
| .click-to-copy:active .feather { | ||
| stroke: #FD5A3B; | ||
| } | ||
| .click-to-copy--confirmation { | ||
| font-size: 0.75em; | ||
| display: inline; | ||
| position: relative; | ||
| bottom: .25rem; | ||
| left: 0; | ||
| font-family: 'Source Sans Pro', sans-serif; | ||
| font-weight: normal; | ||
| color: #5c5956; | ||
| } | ||
| </style> | ||
|
|
||
| <button type="button" class="click-to-copy" id="click-to-copy--{{ value_elem_id }}" aria-label="Copy {{ value_elem_id }} to clipboard"> | ||
| <i data-feather="copy" aria-hidden="true"></i><span id="click-to-copy--confirmation" style="display: none">Value copied!</span> | ||
| </button> | ||
|
|
||
| <script type="text/javascript"> | ||
| $(function () { | ||
| const $copySpan = $('#click-to-copy--{{ value_elem_id }}'); | ||
| const $valueElem = $('#{{ value_elem_id }}'); | ||
|
|
||
| $copySpan.on('click', () => { | ||
| if (!$valueElem.length) return; | ||
| const textToCopy = $valueElem.val() || $valueElem.text(); | ||
| navigator.clipboard.writeText(textToCopy) | ||
| .then(() => { | ||
| const $msg = $('<span class="click-to-copy--confirmation">Value copied!</span>'); | ||
| $copySpan.after($msg); | ||
| setTimeout(() => { | ||
| $msg.remove(); | ||
| }, 3000); | ||
| }) | ||
| .catch(err => { | ||
| console.error('Failed to copy:', err); | ||
| }); | ||
| }); | ||
| }); | ||
| </script> | ||
|
|
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either this is form specific, in which case we should put it in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On my screen this took up quite a lot of space at the top, and it was mostly empty whitespace. I'm not sure what the options are, perhaps just smaller, more compact fonts, or pulling everything onto one line so it takes the least amount of space possible? Could also consider running the notes panel all the way to to the top anyway, to maximise the space for that? (if such a layout is possible)
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| <div class="row"> | ||
| <div class="col-md-8 above_the_fold"> | ||
| <h1 class="above_the_fold--title">{% if obj.es_type == constants.ES_TYPE_JOURNAL %}Journal{% elif obj.application_type == constants.APPLICATION_TYPE_UPDATE_REQUEST %}Update Request{% else %}Application{% endif %}: <span id="above_the_fold__title">{{ obj.bibjson().title }}</span> {% set value_elem_id="above_the_fold__title" %}{% include 'includes/_click_to_copy.html' with context%}</h1> | ||
| <h2><a class="text-muted above_the_fold--url" id="above_the_fold__url" href='{{ obj.bibjson().journal_url }}' target="_blank">{{ obj.bibjson().journal_url }}</a>{% set value_elem_id="above_the_fold__url" %}{% include 'includes/_click_to_copy.html' with context %}</h2> | ||
| <p class="above_the_fold--issns"> | ||
| <span data-feather="book-open"></span> | ||
| {% if obj.bibjson().pissn %} | ||
| P: <span id="above_the_fold__pissn">{{ obj.bibjson().pissn }}</span> | ||
| {% set value_elem_id="above_the_fold__pissn" %}{% include 'includes/_click_to_copy.html' with context %} | ||
| {% if obj.bibjson().eissn %} | ||
| / | ||
| {% endif %} | ||
| {% endif %} | ||
| {% if obj.bibjson().eissn %} | ||
| E: <span id="above_the_fold__eissn">{{ obj.bibjson().eissn }}</span>{% set value_elem_id="above_the_fold__eissn" %}{% include 'includes/_click_to_copy.html' with context %} | ||
| {% endif %} | ||
| </p> | ||
| </div> | ||
| {% if obj and (obj.es_type == constants.LOCK_JOURNAL and obj.is_in_doaj()) %} | ||
| <div class="col-md-4 align-right"> | ||
| <a class="button button--tertiary see_in_doaj" href="{{ url_for('doaj.toc', identifier=obj.id) }}" target="_blank">See this journal in DOAJ</a> | ||
| </div> | ||
| {% endif %} | ||
| </div> |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that introducing a new layout to the form requires a separate test. This is the new layout for the form, so I would just fold the check for this into the general application form tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not a separate test, this would require the team to test the whole application form. We don't change any logic of the form itself, isn't that excessive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing the whole application form when a change has been made to it is a sensible thing to do, as it tests both the feature and any regressions.
The flip-side is that if there's a specific test for this specific aspect of the application form it will take longer to do regression tests in the future, as this could just be part of the application form test rather than an entirely separate test in its own right.