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

Wrap EAD abstract in HTML tag #1165

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Wrap EAD abstract in HTML tag #1165

wants to merge 1 commit into from

Conversation

marlo-longley
Copy link
Contributor

@marlo-longley marlo-longley commented Oct 12, 2022

This makes the UI consistent whether we display data from scopeandcontents or abstract
Thanks to @thatbudakguy for finding this issue + pairing !

@marlo-longley marlo-longley marked this pull request as ready for review October 12, 2022 22:21
@marlo-longley marlo-longley marked this pull request as draft October 12, 2022 22:25
@cbeer
Copy link
Member

cbeer commented Oct 13, 2022

What in the UI is inconsistent whether it's from the abstract or ?scopeandcontents or abstract?

@marlo-longley
Copy link
Contributor Author

marlo-longley commented Oct 13, 2022

If it's a <scopeandcontents> EAD field, <p> tag is getting stored in Solr and returned. <abstract> is just raw unwrapped text stored in Solr. So when we conditionally display one or the other in the same place in the markup, there is a difference in text padding within the same list.

This text came from <scopeandcontents>
Screen Shot 2022-10-13 at 09 14 35

This text came from <abstract>
Screen Shot 2022-10-13 at 09 15 00

@thatbudakguy
Copy link
Member

this has implications for the responsive truncator as well — you can't calculate the height of a bare text node using getBoundingClientRect(), so the logic there only works when the field contents are wrapped in some sort of element.

@thatbudakguy
Copy link
Member

(if there's a "wrap this in an element if it isn't already wrapped" type helper, we could just as easily use that in the component template, I just didn't see anything like that, so this was our solution for now).

@thatbudakguy
Copy link
Member

an example of the truncator issue can be seen in the brief display for the Hemingway collection fixture (you can find it by searching "hemingway"). it has both an <abstract> and a <scopecontent>, and we choose to display the abstract in the brief result. because it's not wrapped in a tag, the truncator can't calculate the height of the content, so the "view more" button doesn't display because truncation is not detected.

<div class="col al-document-abstract-or-scope" data-arclight-truncate="true">
      <div class="content"> This artificial collection includes articles by or about Hemingway, movie posters and photographs, manuscript letters, printed and miscellaneous materials about Ernest Hemingway and his books, diaries of Ernest's uncle, George R. Hemingway, and the organizational records of the Michigan Hemingway Society. </div>
      ...
</div>

Screen Shot 2022-10-21 at 12 25 14

cf. some of the items on the repositories page, which use their <scopecontent> rather than abstract, and so the content is wrapped in a <p> tag. the height can be calculated, and the "view more" toggle displays:

<div class="col al-document-abstract-or-scope truncated" data-arclight-truncate="true">
      <div class="content"><p>Images of students and student activities including track meets, women basketball players, dormitory rooms, picnics and off-campus excursions, commencement activities (including decoration of the arcades as parlors), and theatricals; there are also photographs of campus buildings before and after the 1906 earthquake and of Leland Stanford Jr.'s railroad track on campus. Non-Stanford images include Yosemite and scenes from a European trip. </p>
      ...
</div>

Screen Shot 2022-10-21 at 12 31 20

@thatbudakguy
Copy link
Member

relevant truncator code:

// calculate total scrollable inner height vs. observed outer height
const outerHeight = contentOuter.clientHeight;
const innerHeight = contentInner.map(e => e.scrollHeight).reduce((a, b) => a + b, 0);

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.

None yet

3 participants