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

feat(paginate): re-open PR with #883's work #1090

Closed
wants to merge 2 commits into from

Conversation

Doozers
Copy link

@Doozers Doozers commented Aug 31, 2023

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Aug 31, 2023
@Doozers Doozers marked this pull request as ready for review August 31, 2023 19:03
@Doozers Doozers requested a review from a team as a code owner August 31, 2023 19:03
@Doozers
Copy link
Author

Doozers commented Aug 31, 2023

Hi @tbruyelle tried to modify the previous PR's target branch via github but this created a strange merge commit so as mentionned by @thehowl I will continue here

@tbruyelle
Copy link
Contributor

I think it would be better to migrate your p/demo/md package into /p/demo/ui.

  • md.Link can be removed because it already exists as ui.Link
  • md.EscMDURL should be turned into a private function in p/demo/ui and used internally in ui.Link.Render().

@tbruyelle
Copy link
Contributor

Hi @tbruyelle tried to modify the previous PR's target branch via github but this created a strange merge commit so as mentionned by @thehowl I will continue here

Let's close the previous PR then.

@Doozers
Copy link
Author

Doozers commented Sep 3, 2023

I think it would be better to migrate your p/demo/md package into /p/demo/ui.

  • md.Link can be removed because it already exists as ui.Link
  • md.EscMDURL should be turned into a private function in p/demo/ui and used internally in ui.Link.Render().

yes better this way 👍

Copy link
Contributor

@tbruyelle tbruyelle left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Isma <71719097+Doozers@users.noreply.github.com>
Signed-off-by: Isma <71719097+Doozers@users.noreply.github.com>
@moul moul added this to the 🌟 main.gno.land (wanted) milestone Sep 8, 2023
@jefft0 jefft0 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 8, 2024
@jefft0
Copy link
Contributor

jefft0 commented Oct 11, 2024

Hello @Doozers . There is a new PR for pagination from moul. #2584 . Do you want to close this PR in favor of that one?

@jefft0
Copy link
Contributor

jefft0 commented Oct 22, 2024

Hello @tbruyelle . You approved this pagination PR a year ago. Now there is a new PR from moul with two approvals. #2584 . Should this PR be closed in favor of the new one?

@tbruyelle
Copy link
Contributor

Hello @tbruyelle . You approved this pagination PR a year ago. Now there is a new PR from moul with two approvals. #2584 . Should this PR be closed in favor of the new one?

Well, tbh, right now I don't have the time to dive into this code a year after I first looked at it, but I trust that moul has done a better job, so yeah, let's close this PR.

@thehowl thehowl closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages. review/triage-pending PRs opened by external contributors that are waiting for the 1st review
Projects
Status: Done
Status: 🌟 Wanted for Launch
Status: No status
Development

Successfully merging this pull request may close these issues.

6 participants