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.

Sign up for GitHub

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

Jump to bottom

display quoted notes #944 #1415

Merged
merged 10 commits into from
Aug 23, 2024
Merged

display quoted notes #944 #1415

merged 10 commits into from
Aug 23, 2024

Conversation

bryanmontz
Copy link
Contributor

Issues covered

#944

Description

This PR changes the "Link to note" rendering of links to notes in rendered note cards to show the note itself.

How to test

  1. Navigate to any note that refers to another note.
  2. View the quoted note, tap on it to load.

Screenshots/Video

Before After
old new

@bryanmontz bryanmontz self-assigned this Aug 19, 2024
martindsq
martindsq previously requested changes Aug 19, 2024
View reviewed changes
Copy link
Member

@martindsq martindsq left a comment

Choose a reason for hiding this comment

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

I'm excited to have this on main soon. Quoted notes are great fine for me (I'm not a big fan of the design though, I hope we polish it a little bit afterwards). I'm leaving some comments. I have testing it even more pending.

Nos/Extensions/URL+Extensions.swift Outdated Show resolved Hide resolved
Nos/Models/CoreData/Event+CoreDataClass.swift Outdated Show resolved Hide resolved
Nos/Models/CoreData/Event+CoreDataClass.swift Outdated Show resolved Hide resolved
Nos/Models/CoreData/Event+CoreDataClass.swift Show resolved Hide resolved
joshuatbrown
joshuatbrown reviewed Aug 20, 2024
View reviewed changes
Copy link
Contributor

@joshuatbrown joshuatbrown left a comment

Choose a reason for hiding this comment

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

A few thoughts for now -- I didn't have time for a full review, but what I saw is looking great!

Nos/Models/NoteParser.swift Show resolved Hide resolved
Nos/Models/NoteParser.swift Show resolved Hide resolved
Nos/Models/URLParser.swift Show resolved Hide resolved
Nos/Models/NoteParser.swift Outdated Show resolved Hide resolved
Nos/Models/NoteParser.swift Show resolved Hide resolved
joshuatbrown
joshuatbrown reviewed Aug 21, 2024
View reviewed changes
Copy link
Contributor

@joshuatbrown joshuatbrown 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 looking sooooo great! I love how much better my feed looks with this! A few suggestions around documentation and SwiftGen, but otherwise this is awesome!

Nos/Models/NoteParser.swift Outdated Show resolved Hide resolved
Nos/Models/NoteParser.swift Show resolved Hide resolved
Nos/Views/Modifiers/View+StyledBorder.swift Outdated Show resolved Hide resolved
Nos/Views/Modifiers/View+StyledBorder.swift Outdated Show resolved Hide resolved
NosTests/Models/NoteParserTests.swift Show resolved Hide resolved
Nos/Views/Modifiers/View+StyledBorder.swift Show resolved Hide resolved
@martindsq
Copy link
Member

Thanks for addressing my points @bryanmontz I think we are now closer to finish this ticket.

@bryanmontz
Copy link
Contributor Author

@joshuatbrown and @martindsq I've addressed all the feedback. Anything else I can do to get your approval?

I know we'll need to merge main but I thought we could wait to do that until it's approved to avoid multiple merges.

joshuatbrown
joshuatbrown reviewed Aug 22, 2024
View reviewed changes
Copy link
Contributor

@joshuatbrown joshuatbrown left a comment

Choose a reason for hiding this comment

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

Just one last question. This is looking awesome -- I love it!

Nos/Views/NoteCard.swift Show resolved Hide resolved
joshuatbrown
joshuatbrown approved these changes Aug 22, 2024
View reviewed changes
Copy link
Contributor

@joshuatbrown joshuatbrown left a comment

Choose a reason for hiding this comment

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

Excellent work!

joshuatbrown
joshuatbrown approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@joshuatbrown joshuatbrown left a comment

Choose a reason for hiding this comment

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

Latest updates look great!

@joshuatbrown joshuatbrown dismissed martindsq’s stale review August 23, 2024 13:38

comments were addresses; no request for changes in the review 2 days ago

@joshuatbrown joshuatbrown added this pull request to the merge queue Aug 23, 2024
Merged via the queue into main with commit a1b1238 Aug 23, 2024
4 checks passed
@joshuatbrown joshuatbrown deleted the bdm/944-quoted-notes branch August 23, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshuatbrown joshuatbrown joshuatbrown approved these changes

@mplorentz mplorentz Awaiting requested review from mplorentz mplorentz is a code owner

@martindsq martindsq Awaiting requested review from martindsq

Assignees

@bryanmontz bryanmontz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bryanmontz @martindsq @joshuatbrown @mplorentz