-
Notifications
You must be signed in to change notification settings - Fork 82
setup a11y info pages on all three themes #4982
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: master
Are you sure you want to change the base?
Conversation
Just realised the clean theme journal footer has the accessibility link and contact link swapped around. Adding another commit to fix this and make it consistent with the order everywhere else. |
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.
The pages are great. I have a few suggestions about the data model and logic of the press-dependent setting mechanism.
src/core/views.py
Outdated
# Non-press-dependent settings are always included | ||
filtered_settings.append(setting) | ||
|
||
settings = filtered_settings |
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 so sure the settings should be invisible at the journal level. We already often have the problem that people don't understand why they can't see something that other people tell them should be there. This would re-create that problem in a new place unless someone knows that these settings are interrelated at the press and journal level. Staff might not always know how to work this feature, or communicate it to the journal editors.
What if instead of disappearing, the settings were read-only and could not be edited?
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 did wonder about this for some time while working on the branch. My feeling was that in this specific instance, no or at least not yet. because I think the accessibility bit needs to be rolled out more slowly. Likely first to Press managers, so they can think about their part, and whether they want journal staff to see this or not. And only then to make it visible.
What I want to avoid is journal staff seeing the setting, and it's also the first time they've seen a setting that is not active. Particularly as by making this an 'a11y' group setting, it appears by default at the start of the all settings list.
Longer term, yes, I do think they should be visible but inactive and signposted as 'needs X setting on press'. But I don't want to introduce both inactive settings and accessibility page at the same time.
}, | ||
"editable_by": [ | ||
"journal-manager" | ||
] |
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.
Just to record what we discussed off GitHub, the is_press_dependent
field will need to be extended to all items in the JSON file, for consistency and maintainability.
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 looked at doing this - but the file is not consistent as is - the setting attributes aren't always in the same order - so making it consistent feels like a stand alone task.
02ec4a6
to
d6f408c
Compare
closes #4694