Individual Transcript Page #52
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
0tuedon
force-pushed
the
tuedon/individual-transcript
branch
from
fda05c4 to
228bd83
Compare
0tuedon
force-pushed
the
tuedon/individual-transcript
branch
from
1af2ab4 to
9290f83
Compare
|
Looks good!! UI/UX Reviewissue: Empty Navigation paneldescription: The navigation panel should not render when a transcript doesn't have chapters. It should still occupy the spacing. issue: Styling for timestamped speakersdescription: There's no styling for speakers with timestamps. You mentioned working on this, is this a blocker? issue: Scrolling on transcript contentdescription: There is a scroll on transcript content, is this what we want? I assume it should be a full page scroll depending on the transcript height. And if it is scrollable then it should not scroll more than the page itself i.e we shouldn't have two scrollbars — one on the page and the other on transcript tab. This screenshot applies to the 3 issues above issue: Chapter Navigationdescription:
cc: @0tuedon |
I think this is just a regex failure will check 
|
Unfortunately, we have transcripts that may not follow our pattern for speaker attribution. We don't need to support them, it's okay if we don't have this type of highlighting for them. As soon as we have this working we will change the transcripts so that they follow the same format for speakers attribution. |
Emmanuel-Develops
force-pushed
the
staging
branch
from
4797367 to
a70086e
Compare
0tuedon
force-pushed
the
tuedon/individual-transcript
branch
from
4fc791b to
dd97557
Compare
0tuedon
force-pushed
the
tuedon/individual-transcript
branch
from
dd97557 to
803c183
Compare
0tuedon
force-pushed
the
tuedon/individual-transcript
branch
from
803c183 to
2de5ed6
Compare
This link isn't clickable |
0tuedon
force-pushed
the
tuedon/individual-transcript
branch
from
11543c0 to
0dfd6f7
Compare
|
There's a glitch on chapter navigation chapter_navigation_glitch_trim2.mp4cc: @0tuedon |
0tuedon
force-pushed
the
tuedon/individual-transcript
branch
from
0dfd6f7 to
5788408
Compare
0tuedon
force-pushed
the
tuedon/individual-transcript
branch
from
5788408 to
b0d34e1
Compare
0tuedon
force-pushed
the
tuedon/individual-transcript
branch
from
b0d34e1 to
a1a5790
Compare
0tuedon
force-pushed
the
tuedon/individual-transcript
branch
from
a1a5790 to
a5b37f2
Compare
0tuedon
force-pushed
the
tuedon/individual-transcript
branch
from
a5b37f2 to
d4eb6f7
Compare
There was a problem hiding this comment.
Page looks good!
I left quite a few comments regarding implementation details and code readability and maintainability.
Additionally, it seems to me that the current chapter navigation might slow down the page loading, but I'll have to do better analysis in order to verify, not blocking tho.
| return notFound(); | ||
| } | ||
|
|
||
| let data: any = allSources; |
There was a problem hiding this comment.
is there a type for this? we shouldn't use any. Also, more descriptive names help everybody. "data" is too generic.
There was a problem hiding this comment.
This is a really tricky issue, i did try to create some types that didn't end up working as perfect as we would want it.
Issues mainly occurred on this line
data = data[crumb as keyof typeof allSources];
Also, name change will be on the next commit
There was a problem hiding this comment.
it looks to be that the BreadCrumbs should be using BaseBreadCrumbs instead of having duplicate code.
btw, "breadcrumbs" is one word, so it should be "Breadcrumbs"
There was a problem hiding this comment.
it looks to be that the BreadCrumbs should be using BaseBreadCrumbs instead of having duplicate code.
Yh, I and Emma didn't do a refactor there just because we wanted to get it out, regardless it will be in my next commit.
btw, "breadcrumbs" is one word, so it should be "Breadcrumbs";
Actually both are correct, it could be breadcrumbs or bread crumbs.
| export function createContentSlug(name: string): string { | ||
| return name | ||
| .toLowerCase() | ||
| .replace(/\s+/g, "-") // Replace spaces with hyphens | ||
| .replace(/[^\p{L}\p{N}\-_]+/giu, "") // Remove all non-alphanumeric characters except hyphen | ||
| .replace(/\-\-+/g, "-") // Replace multiple hyphens with a single hyphen | ||
| .replace(/^-+/, "") // Trim hyphens from the start | ||
| .replace(/-+$/, ""); // Trim hyphens from the end | ||
| } |
There was a problem hiding this comment.
this method looks the same as createSlug above.
There was a problem hiding this comment.
They are different, and the reason for this was our language issue.
createSlug is being used in a lot of place and I did not want to change the implementation of our createSlug since it works where it's being used
This regex /[^\p{L}\p{N}\-_]+/giu will leave characters like "α" and other languages character. that's the difference between the two.
|
@kouloumos can you review the latest changes regarding code readability and maintainability |
|
@0tuedon please resolve conflicts |
Emmanuel-Develops
merged commit 52c8937
into
bitcointranscripts:staging
* refactor setup algorithm * chore: remove slug from transcript card * feat: Individual transcript page render * feat: icons and tab functionality * feat: individual transcript page functionality * fix: markdown # headings * fix: spacing between link and design nit * feat: Individual transcript page render * feat: icons and tab functionality * feat: individual transcript page functionality * fix: markdown # headings * fix: h1,h2 check and code tag adjustment * chore(version): updated bitcoin-dev-project version * fix(refactor): code readbility --------- Co-authored-by: IgboPharaoh <[email protected]> Co-authored-by: Emmanuel Itakpe <[email protected]>
* Refactor: source data structure to account for languages and metadata (#57) * Add boss banner (#63) * chore: add source to transcript card (#60) * Individual Transcript Page (#52) Co-authored-by: 0tuedon <[email protected]> Co-authored-by: IgboPharaoh <[email protected]>
This PR will close #50
Transcript MetaCard
Table of Contents
Transcript Tab
"Speaker: HH:MM:SS"followed by a newline.issue: