Skip to content
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

Descriptive terms #275

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Descriptive terms #275

wants to merge 2 commits into from

Conversation

wkdewey
Copy link
Contributor

@wkdewey wkdewey commented Nov 21, 2022

See #239 which got closed because it was based on a branch I deleted, reopening. I think there were some outstanding issues left on it.

Fixes #213

@wkdewey
Copy link
Contributor Author

wkdewey commented Nov 30, 2022

Reset the branch to only have the relevant commit

Copy link
Member

@techgique techgique left a comment

Choose a reason for hiding this comment

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

I think I found an oversight in the HTML logic for this

@@ -36,6 +36,7 @@ def metadata_create_field_link(api_field, item)
item_label = value_label(api_field, item)
link_to item_label, prefix_path(route_path, search_params),
rel: "nofollow"
"<dd>#{link}</dd>"
Copy link
Member

@techgique techgique Dec 20, 2022

Choose a reason for hiding this comment

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

There's no link variable in scope here. The return of link_to would have to be assigned to one first. But I think there's another oversight here. This is the only place we have <dd> and </dd> tags. This code here only affects output when the metadata method isn't called with link: false, but it is in many places. So they won't be wrapping non-link metadata info as the non-centralized <li> tags in the template were before. I think we actually want to just remove this line here and handle output of the <dd> tags in the .map above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed as suggested

@@ -22,7 +22,7 @@ def metadata(res, label, api_field, link: true, separator: " | ")
end
end
html << dataArray
.map { |i| "<span>#{i}</span>" }
.map { |i| "#{i}" }
Copy link
Member

Choose a reason for hiding this comment

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

I think changing this to

.map { |i| "<dd>#{i}</dd>" |

is what we want to replace all of the old <li> tags with <dd> tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed as suggested

@wkdewey
Copy link
Contributor Author

wkdewey commented Dec 20, 2022

Testing this on whitman, I noticed it caused some styling changes. Maybe that needs to be handled in the specific project rather than Orchid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

descriptive term list rather than li for metadata
2 participants