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

component/transcript-search #61

Merged

Conversation

tohuynh
Copy link
Collaborator

@tohuynh tohuynh commented Jun 26, 2021

Pull request recommendations:

I changed TranscriptItem a bit. I thought the Video clip and Transcript buttons were too cluttered on mobile so I decided to change them to icon(⏵ and →) buttons (with a tooltip that displays Play video clip and Go to transcript on hover).

I introduced some shared types Sentence and Word (to make writing code easier), which is pretty much a copy of what is used in transcript model.

Then the list of sentences component is pretty much a copy of EventTranscriptSearch using react-virtualized.

  • Provide relevant tests for your feature or bug fix.
  • Provide or update documentation for any feature added by your pull request.
    Two stories on storybook. One for a shortlist of sentences and the other for a longer list.
    Thanks for contributing!

@tohuynh tohuynh self-assigned this Jun 26, 2021
Copy link
Member

@evamaxfield evamaxfield left a comment

Choose a reason for hiding this comment

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

This is awesome! 🥳 Approving because all of my questions are not specifically related to this PR. Also hoping @hawkticehurst can take a look.

Minor comments:

  1. Totally agree with the icon changes (the transcript one... I wish looked maybe a bit different but idk what to do there. In general I like it overall much more though.

  2. Have you looked at the "small mobile" view for "transcript item" in general in Storybook. Seems like we may need to shrink the width of the container / component in that view or just go by 100% width after a breakpoint. But we should probably think more in-depth about search experience on small devices in general... That's a much larger discussion and so we should have that discussion after v3 launch so no changes needed now except maybe the width adjustment change?

  3. Weird idea, and maybe something to just add an issue for to solve later or for someone else to take on at the hackathon or something. But it may be nice to come up with a system for substringing the text based off of the users query to reduce the amount of words in the resulting sentence. I.e. take the "Transcript Item w/ Search Query" storybook example. That is a long sentence, what if, when searching, the user types in a couple of words and just like the other event card system where we substring and find the prior N and tailing N words around the query result. So the sentence is: "Hi, I am hear today to discuss the addition of the proposed 35th avenue bike lane and how I think it will be a massive benefit to the whole community." That is a short example sentence, but using it, we say someone searches for "bike lane" and we have it parametrized to give the prior and tailing 7 words, the transcript search would show: "... the addition of the proposed 35th avenue bike lane and how I think it will be..." This is totally just a random thought and maybe something to add later after we deploy v3 and see all of this in action.

  4. Somewhat concered about having the picture source be remote for all of these items... Idk how the web works enough... will that picture automatically be cached and reused across the board or does every element with it make a request?

src/components/Details/TranscriptItem/TranscriptItem.tsx Outdated Show resolved Hide resolved
src/components/Details/TranscriptItem/TranscriptItem.tsx Outdated Show resolved Hide resolved
@tohuynh
Copy link
Collaborator Author

tohuynh commented Jun 26, 2021

  1. Thanks I didn't see that. I'll remove the decorator that limits the container of the transcript item to 320px in width for now.
  2. Makes sense. I'll make a GH issue for it.
  3. I think your browser would request it once and then cache.

@hawkticehurst
Copy link
Collaborator

Some immediate thoughts before giving a full review a little later today.

  1. In the small mobile view can we change the ordering, so that the content is stacked in a column orientation versus a row?

Screen Shot 2021-07-01 at 9 24 56 AM

  1. In a similar vein to what @JacksonMaxfield said could we also potentially set the max-height of the search results container to fill the remaining height of the screen? That's a lot of white space at the bottom of these two mobile views.

Screen Shot 2021-07-01 at 9 25 03 AM

Screen Shot 2021-07-01 at 9 25 21 AM

Copy link
Collaborator

@hawkticehurst hawkticehurst left a comment

Choose a reason for hiding this comment

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

Beyond the comment I left the other day, looks good to me!

@tohuynh
Copy link
Collaborator Author

tohuynh commented Jul 7, 2021

In the small mobile view can we change the ordering, so that the content is stacked in a column orientation versus a row?

Yes

In a similar vein to what @JacksonMaxfield said could we also potentially set the max-height of the search results container to fill the remaining height of the screen? That's a lot of white space at the bottom of these two mobile views.

It's from the fixed 600px height decorator I have for these stories. Though I think that container for the meeting page should determine the height for this component, as the video player component (at least on desktop) will dictate how much height the search transcript will get.

A weird thing, I'm using ⏵for the play video clip button, and it looks like it's not showing up for you.

@tohuynh
Copy link
Collaborator Author

tohuynh commented Jul 7, 2021

@hawkticehurst I'm using U+23F5 ⏵ for the Play video clip button and it's showing up for you as those horizontal bars?

@evamaxfield
Copy link
Member

@tohuynh I am not seeing any issues on the icon stuff so going to merge this. If @hawkticehurst responds in a bit with what was going on then a patch later I think.

@evamaxfield evamaxfield merged commit 02523fc into CouncilDataProject:main Jul 10, 2021
@tohuynh tohuynh deleted the component/transcript-search branch July 11, 2021 02:28
@hawkticehurst
Copy link
Collaborator

Yeah I don't know what's up, but both on my phone and computer (both Apple devices) I just see this vertical bar thing instead of a play button.

Screen Shot 2021-07-14 at 7 29 38 PM

@tohuynh
Copy link
Collaborator Author

tohuynh commented Jul 15, 2021

@hawkticehurst Do you also see the bars here under Symbol: https://en.wikipedia.org/wiki/Media_control_symbols

@hawkticehurst
Copy link
Collaborator

Hmmm I do not see bars interestingly 🤔

Screen Shot 2021-07-14 at 8 03 47 PM

@tohuynh
Copy link
Collaborator Author

tohuynh commented Jul 15, 2021

Sorry, I meant under Unicode, (the one under Symbol is an SVG). I think you do see the bars, under U+23F5. Whatever it is, you don't see the play symbol.

@hawkticehurst
Copy link
Collaborator

Ahh I see, yeah in that case it does not seem to work––it seems to just display the empty square/box that is shown when macOS doesn't know what to render.

@evamaxfield
Copy link
Member

@tohuynh should we just use the SVG versions?

@hawkticehurst
Copy link
Collaborator

I'll chime in to say I think we definitely should always go for SVG versus unicode. There's always going to be inconsistencies across devices/operating systems

@hawkticehurst
Copy link
Collaborator

Also, generally SVG > any other potential format (whenever possible) because vector-based graphics are just so flexible

@tohuynh
Copy link
Collaborator Author

tohuynh commented Jul 15, 2021

Then let's use the svg from heroicons: https://heroicons.com/?

play and arrow-right?

@evamaxfield
Copy link
Member

what about:

"play" for jump to point in video and "document-text" for jump to point in transcript

@tohuynh
Copy link
Collaborator Author

tohuynh commented Jul 15, 2021

document-text works too. If the SVG isn't readily apparent on the meaning we still have the tooltip on hover (Jump to sentence in transcript).

@hawkticehurst
Copy link
Collaborator

Yeah, "play" and "document-text" icons + tooltip seems great to me!

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.

Transcript search card and viewer for the event detail page
3 participants