-
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
Open
amdomanska
wants to merge
18
commits into
develop
Choose a base branch
from
feature/4101_editorial_review_layout
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 6 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
6ed695b
add information above the fold, adjust layout for shorter screens, cr…
amdomanska 97521a0
add testdrive and testbook files
amdomanska bd2fb5a
update the functional tests
amdomanska 8799d76
fix small layout issues, improve functional tests
amdomanska 8c51ae1
fix templates
amdomanska 4d6a6ea
small change in the testdrive
amdomanska f261d05
reverse seamless change
amdomanska 80fa5d1
fix naming, initialise jquery sooner
amdomanska cf34f2f
fix base, add journal type to const
amdomanska c1c24a6
add journal type to const
amdomanska 168549a
refactor
amdomanska 66487e9
change block to variable to remove repetition
amdomanska afcd0bf
Merge branch 'develop' into feature/4101_editorial_review_layout
amdomanska 4199746
Code refactoring
amdomanska 720db8c
Display and style confirmation message
amdomanska b15dc58
Include the functional tests in the standard application form tests
amdomanska 4d4e364
remove obsolete testdrive
amdomanska d7c512f
Merge branch 'develop' into feature/4101_editorial_review_layout
Steven-Eardley File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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; | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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; | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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"} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| <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 waitForJQuery() { | ||
amdomanska marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if (window.jQuery) { | ||
| $(function() { | ||
| var $copySpan = $('#click-to-copy--{{ value_elem_id }}'); | ||
| var $valueElem = $('#{{ value_elem_id }}'); | ||
|
|
||
| $copySpan.on('click', function() { | ||
| if (!$valueElem.length) return; | ||
|
|
||
| var textToCopy = $valueElem.val() || $valueElem.text(); | ||
|
|
||
| navigator.clipboard.writeText(textToCopy).then(function() { | ||
| var $msg = $('<span class="click-to-copy--confirmation">Value copied!</span>'); | ||
| $copySpan.after($msg); | ||
|
|
||
| setTimeout(function() { | ||
| $msg.remove(); | ||
| }, 3000); | ||
| }).catch(function(err) { | ||
| console.error('Failed to copy:', err); | ||
| }); | ||
| }); | ||
| }); | ||
| } else { | ||
| setTimeout(waitForJQuery, 50); // check again in 50ms | ||
| } | ||
| })(); | ||
| </script> | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.