Skip to content

#3906 un-nest labels and inputs in themes#3908

Merged
joemull merged 2 commits into
masterfrom
3906-bugfix_form_field_missing_label
Mar 14, 2024
Merged

#3906 un-nest labels and inputs in themes#3908
joemull merged 2 commits into
masterfrom
3906-bugfix_form_field_missing_label

Conversation

@StephDriver

@StephDriver StephDriver commented Jan 24, 2024

Copy link
Copy Markdown
Member

all file search for labels which nested their input and only two were found, both in theme templates marked as depreciated in 1.5.1.

Labels elsewhere looked un-nested, and hence avoiding this issue.

Closes #3906

ajrbyers
ajrbyers requested changes Feb 15, 2024
View reviewed changes

@ajrbyers ajrbyers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is okay, does the display remain the same after the change?

Comment thread src/themes/OLH/templates/elements/journal/article_filter_form.html Outdated
@StephDriver StephDriver requested a review from ajrbyers March 5, 2024 15:02
joemull
joemull reviewed Mar 14, 2024
View reviewed changes

@joemull joemull left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thought I'd mention this template is no longer used, to my knowledge. I added this deprecation note recently when I switched this interface to using a class-based view:

https://github.com/BirkbeckCTP/janeway/blob/9fcbbe87b917bf3c4ce1d64b85d579f7c0634069/src/themes/OLH/templates/elements/journal/article_filter_form.html#L1

Maybe I should have just deleted the template? I wasn't confident doing so without more front-end tests in place.

@ajrbyers

ajrbyers commented Mar 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Just thought I'd mention this template is no longer used, to my knowledge. I added this deprecation note recently when I switched this interface to using a class-based view:

https://github.com/BirkbeckCTP/janeway/blob/9fcbbe87b917bf3c4ce1d64b85d579f7c0634069/src/themes/OLH/templates/elements/journal/article_filter_form.html#L1

Maybe I should have just deleted the template? I wasn't confident doing so without more front-end tests in place.

I think its safe to remove it in 1.6. Any break in rendering of the article list view will be picked up by https://github.com/BirkbeckCTP/janeway/blob/9fcbbe87b917bf3c4ce1d64b85d579f7c0634069/src/journal/tests/test_journal_frontend.py#L98-L112.

@joemull joemull mentioned this pull request Mar 14, 2024
Open
@joemull

joemull commented Mar 14, 2024

Copy link
Copy Markdown
Member

@ajrbyers Brilliant! I've added an issue for it.

@joemull joemull merged commit 343064b into master Mar 14, 2024
@joemull joemull deleted the 3906-bugfix_form_field_missing_label branch March 14, 2024 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ajrbyers ajrbyers ajrbyers approved these changes
@joemull joemull joemull approved these changes
@mauromsl mauromsl Awaiting requested review from mauromsl

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

form field missing a label

3 participants

@StephDriver @ajrbyers @joemull