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

Nested headings for Transcript Page #75

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

0tuedon
Copy link
Collaborator

@0tuedon 0tuedon commented Dec 28, 2024

This PR adds support for nested transcript navigation chapters and closes issue #64 .
You can Test with this link

Copy link

vercel bot commented Dec 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
registry ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2025 1:50pm

src/utils/index.ts Outdated Show resolved Hide resolved
src/components/explore/TranscriptContentPage.tsx Outdated Show resolved Hide resolved
@IgboPharaoh
Copy link
Collaborator

IgboPharaoh commented Jan 9, 2025

Hello @0tuedon can you please rebase your branch to the latest on staging to resolve merge conflicts

@0tuedon
Copy link
Collaborator Author

0tuedon commented Jan 9, 2025

This new commit cb3712c is meant to changed the complex data structure we had for Content Page and Content Grouping. This should resolve comments made by @kouloumos and @Emmanuel-Develops.

There might still be some nit fixes and I will need extra eyes on this.
cc @IgboPharaoh @Emmanuel-Develops @kouloumos

src/utils/index.ts Outdated Show resolved Hide resolved
src/components/explore/AlphabetGrouping.tsx Outdated Show resolved Hide resolved
src/components/explore/ContentGrouping.tsx Outdated Show resolved Hide resolved
src/components/explore/ContentGrouping.tsx Outdated Show resolved Hide resolved
src/components/explore/ContentGrouping.tsx Outdated Show resolved Hide resolved
@@ -32,14 +32,12 @@ const TranscriptContentPage: FC<ITranscriptContentPage> = ({
linkName,
type,
}) => {
// [{name: "A", slug:"a", nested: [{name: "ABC", count: 1}]}]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can remove commented out code here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was to explain the format and dataStructure the functions below will return

Copy link
Collaborator

@IgboPharaoh IgboPharaoh Jan 14, 2025

Choose a reason for hiding this comment

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

Ohhhh. Please can you add a comment explaining that ?

color: #000;
font-size: 1rem
scroll-margin-top: 100px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this is the only line of change, this should be what is reflected in this change but I think your prettier formatting options added some extra changes

@IgboPharaoh
Copy link
Collaborator

IgboPharaoh commented Jan 14, 2025

While reviewing the PR, I tested the links in the example transcript tagged in the PR description and I noticed that clicking on them don't navigate you to a new page while retaining the bitcoin transcripts in its own tab but you're routed to the page inside the existing transcript page hence taking you out of the app.

Is this the expected behaviour we want when we click on external links when reading a transcript ? @0tuedon @Emmanuel-Develops @kouloumos

My concern is when you route a user inside the same tab, there is a tendency they will continue to explore other materials without going back to the app. But when the link is opened in a new tab, you are sure after the user has explored new information from the external link, they can comeback to reading the transcript as the tab still exists and no new information has been added to url history state.

@0tuedon
Copy link
Collaborator Author

0tuedon commented Jan 14, 2025

While reviewing the PR, I tested the links in the example transcript tagged in the PR description and I noticed that clicking on them don't navigate you to a new page while retaining the bitcoin transcripts in its own tab but you're routed to the page inside the existing transcript page hence taking you out of the app.

Is this the expected behaviour we want when we click on external links when reading a transcript ? @0tuedon @Emmanuel-Develops @kouloumos

Oh yh I agree this can be done, not sure if I asked the question before (looks familiar) waiting for confirmation from @kouloumos and @Emmanuel-Develops .

@Emmanuel-Develops
Copy link
Collaborator

Oh yh I agree this can be done, not sure if I asked the question before (looks familiar) waiting for confirmation from

its a nit we can address in dev-design call.

@0tuedon I noticed there is no hover state for the nested links

@0tuedon
Copy link
Collaborator Author

0tuedon commented Jan 14, 2025

@0tuedon I noticed there is no hover state for the nested links

Yh messaged tobi last week let me remind him

Copy link
Collaborator

@Emmanuel-Develops Emmanuel-Develops left a comment

Choose a reason for hiding this comment

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

Looks good, left some few nits

src/components/explore/GroupedTranscriptContent.tsx Outdated Show resolved Hide resolved
src/components/explore/TranscriptContentPage.tsx Outdated Show resolved Hide resolved
src/components/explore/ContentGrouping.tsx Outdated Show resolved Hide resolved
src/components/explore/AlphabetGrouping.tsx Outdated Show resolved Hide resolved
@Emmanuel-Develops Emmanuel-Develops merged commit dd24d3c into bitcointranscripts:staging Jan 16, 2025
2 checks passed
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.

3 participants