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

update-profile request #2632

Closed
wants to merge 21 commits into from
Closed

update-profile request #2632

wants to merge 21 commits into from

Conversation

friedger
Copy link
Contributor

@friedger friedger commented Aug 23, 2022 •
edited
Loading

This PR

  • adds support for update profile requests. These requests are sent from any app and requests that the public profile is updated. The profile is shown to the user before the public profile of the user is updated.
  • fixes Support updating the public profile #2544

image

cc/ @kyranjamie @fbwoolf @beguene @He1DAr

@vercel
Copy link

vercel bot commented Aug 23, 2022 •
edited
Loading

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

Name Status Preview Updated
stacks-wallet-web ✅ Ready (Inspect) Visit Preview Oct 26, 2022 at 11:12PM (UTC)

@vercel vercel bot temporarily deployed to Preview August 23, 2022 23:02 Inactive
@friedger friedger mentioned this pull request Aug 24, 2022
Closed
@markmhendrickson
Copy link
Collaborator

@friedger any way you could provide a test app of sorts that helps us QA this with various inputs?

@markmhendrickson
Copy link
Collaborator

Also I see this is in "Draft" – do you prefer we wait on our review until it's out of that state?

@friedger
Copy link
Contributor Author

I would like to get early feedback. This is in a state that works.

To get a working app we would need connect support, wouldn't we?

kyranjamie
kyranjamie reviewed Sep 5, 2022
View reviewed changes
Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

Sorry for the delay leaving feedback @friedger. Left some high level comments, I think there's some duplication/tidying we'd want, but haven't mentioned atm as it's in draft.

Maybe @markmhx can speak to where this comes on our priorities to get merged.

src/app/pages/update-profile-request/update-profile-request.tsx Outdated

// Temporary workaround to avoid pattern of pulling search params directly
// into an atom, rather than using the tooling provided by our router library
useSetAtomUpdateProfileRequestToken(requestToken);
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 a new feature, we shouldn't need this updater logic

friedger
friedger commented Sep 5, 2022
View reviewed changes
Copy link
Contributor Author

@friedger friedger left a comment

Choose a reason for hiding this comment

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

I hope to get this merged sooner than later :-)

@friedger
Copy link
Contributor Author

friedger commented Sep 5, 2022

Wallet-sdk PR is here: hirosystems/stacks.js#1351

@markmhendrickson
Copy link
Collaborator

@friedger Want to update this PR based on the comments above and let us know when it's ready for another review?

@kyranjamie
Copy link
Collaborator

@friedger it'd be great if we could add integration tests for this flow. Happy to run you through how to write/run these.

@friedger
Copy link
Contributor Author

@kyranjamie thank you. I'll contact you for a date.

@markmhx I try to get this updated as quickly as possible. 😁

@vercel vercel bot temporarily deployed to Preview September 30, 2022 14:49 Inactive
@vercel vercel bot temporarily deployed to Preview September 30, 2022 16:26 Inactive
@vercel vercel bot temporarily deployed to Preview September 30, 2022 16:33 Inactive
@vercel vercel bot temporarily deployed to Preview September 30, 2022 19:31 Inactive
@vercel vercel bot temporarily deployed to Preview September 30, 2022 20:47 Inactive
@markmhendrickson markmhendrickson removed the request for review from fbwoolf October 4, 2022 08:02
@markmhendrickson
Copy link
Collaborator

@friedger I see you've been active on this PR but it's not clear it's ready for re-review. When it is, please reassign @kyranjamie as a reviewer and we'll triage 🙏

@friedger
Copy link
Contributor Author

friedger commented Oct 4, 2022

@markmhx In order to get integration tests going we need support by connect: hirosystems/connect#274

@vercel vercel bot temporarily deployed to Preview October 5, 2022 14:16 Inactive
@vercel vercel bot temporarily deployed to Preview October 5, 2022 20:03 Inactive
@friedger
Copy link
Contributor Author

friedger commented Oct 5, 2022

image

Profile updating via test app works.

Next:

@vercel vercel bot temporarily deployed to Preview December 1, 2022 15:07 Inactive
friedger and others added 20 commits December 1, 2022 16:31
@friedger
feat: add support for update-profile request
2f98b7a
@friedger
feat: fix import
ad7e170
@friedger
fix: use correct ui components
041ff62
@friedger
fix: use whenStxChainId, use UI components
70bec8f
@kyranjamie @friedger
chore: upgrade stacks.js v5
2c95e2b
@fbwoolf @friedger
fix: path url trailing slash
1106fa3
@friedger
fix: revert accidential changes
60b1f36
@friedger
chore: restore test files for profile updates
8111ccb
@friedger
fix: fix resolution of @stacks/profile to 6.0.0
2c8018b
@friedger
fix: restore event handler in content-script, apply prettier
42fa2b1
@friedger
fix: remove waitForNavigation
ba27d55
@friedger
fix: revert wrong rebase changes
21d6c0f
@friedger
feat: add support for update-profile request
3a2ef82
@friedger
feat: fix import
46b2896
@kyranjamie @friedger
chore: upgrade stacks.js v5
a04f147
@fbwoolf @friedger
fix: path url trailing slash
b1572b7
@friedger
fix: revert accidential changes
084b019
@friedger
chore: restore test files for profile updates
b952710
@friedger
fix: restore event handler in content-script, apply prettier
1063db3
@friedger
fix: remove duplicate imports
d48458f
@friedger friedger force-pushed the feat/update-profile branch from 77a4000 to d48458f Compare December 1, 2022 15:40
@vercel vercel bot temporarily deployed to Preview December 1, 2022 15:42 Inactive
@markmhendrickson
Copy link
Collaborator

@friedger it looks like we have one last lint check to resolve?

@friedger
Copy link
Contributor Author

friedger commented Dec 6, 2022

@markmhx What does the lint rule mean? What needs to be changed? Is this something related to the project setup? I don't see how I can fix this.

error no-non-package-json: src/shared/actions/finalize-profile-update.ts → node_modules/@stacks/profile/dist/esm/index.js

@kyranjamie
Copy link
Collaborator

This error means you're using @stacks/profile without it being listed in the package JSON

@vercel vercel bot temporarily deployed to Preview December 6, 2022 15:41 Inactive
@kyranjamie
Copy link
Collaborator

Closing in favour of #2829

@kyranjamie kyranjamie closed this Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyranjamie kyranjamie kyranjamie left review comments

@janniks janniks janniks left review comments

@edu-stx edu-stx edu-stx left review comments

@fbwoolf fbwoolf fbwoolf approved these changes

@markmhendrickson markmhendrickson Awaiting requested review from markmhendrickson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support updating the public profile
8 participants
@friedger @markmhendrickson @kyranjamie @CLAassistant @fbwoolf @paradigma-cl @janniks @edu-stx