-
Notifications
You must be signed in to change notification settings - Fork 79
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(new-links): Parse url data and display #11619
Conversation
Jenkins BuildsClick to see older builds (41)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
I added one comment about the functions probably not belonging in the main module, but I don't really have a better option form the top of my head.
I had two questions though:
- does the "Start chat" work with the new urls too? Like pasting the profile url of someone should open their chat or the contact request popup
- Does the community data in the unfurl update if the community info changes? For example, if the community name changes after it unfurled, does it change to use the new name?
src/app/modules/main/module.nim
Outdated
jsonObj["displayName"] = %* contactData.displayName | ||
jsonObj["description"] = %* contactData.description | ||
jsonObj["publicKey"] = %* contactData.publicKey | ||
return $jsonObj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put those in the urls_manager
instead?
I think we just need to expose it to the QML using setRootContextProperty
.
Though I think it can't have access to services.
Anyway, maybe someone has an idea where we could put them? Putting them in the main module works, it just feels like it's a bit random
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a url service and mvc makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so probably, I should move generators to new url service as well? otherwise, its service with just one method. @jrainville @MishkaRogachev what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I do believe that urls could become massive in the future
unfortunately, i forgot about start chat... will do. |
5621802
to
43ae52c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job
src/app/modules/main/controller.nim
Outdated
@@ -22,6 +22,7 @@ import ../../../app_service/service/wallet_account/service as wallet_account_ser | |||
import ../../../app_service/service/token/service as token_service | |||
import ../../../app_service/service/network/service as networks_service | |||
import ../../../app_service/service/collectible/service as collectible_service | |||
import ../../../app_service/service/general/service as general_service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one is no longer needed
ui/imports/utils/Utils.qml
Outdated
} | ||
|
||
let communityDataString = sharedUrlsModuleInst.parseCommunitySharedUrl(link) | ||
return JSON.parse(communityDataString) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need a try/catch
?
43ae52c
to
fa01c41
Compare
Currently community link previews are not shown in chat. I commented them out to implement after this: @jrainville do you think we can merge this without working link previews and I will take care of them when backend is ready? |
Sure, as long as clicking the link takes you to the community |
f19e844
to
93d4435
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great new service and mvc for shared links
error "error: ", procName="backupData", errName = e.name, errDesription = e.msg | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray empty line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty line at the end of a file is good idea, isn't it?
I don't remember the actual reason, but even GitHub marks it with a red icon 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure VSCode has something like that too
93d4435
to
bc3abdc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and it works fine. As mentioned before the preview is not available but clicking on the link takes the user to the community. User links also work :)
bc3abdc
to
8d53a27
Compare
8d53a27
to
156955f
Compare
Fixes: #10852
What does the PR do
Unfurling new urls formats
Affected areas
Invitations
StatusQ checklist
Screenshot of functionality (including design for comparison)