-
Notifications
You must be signed in to change notification settings - Fork 213
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
Try open matrix: links before showing clients #223
base: main
Are you sure you want to change the base?
Conversation
src/Link.js
Outdated
return null; | ||
} | ||
const identifier = this.identifier.substring(1); | ||
const suffix = this.eventId ? `/e/${this.eventId.substring(1)}` : ""; |
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.
need to url-encode event id and identifier here I think
src/RootViewModel.js
Outdated
const matrixUrl = this.link.toMatrixUrl() | ||
if (matrixUrl) { | ||
this.hidden = true; | ||
setTimeout(() => { |
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.
perhaps pass setTimeout
in as a view model option in case we want to unit test these one day
src/open/OpenLinkViewModel.js
Outdated
_tryLink() { | ||
const matrixUrl = this._link.toMatrixUrl() | ||
if (matrixUrl) { | ||
this.tryingLink = true; |
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.
don't you need an emitChange
here?
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 assumed not, since this is called from the constructor
, and the views are only created afterwards. I don't think any handlers can be registered at the time of the constructor call? Maybe I'm misunderstanding.
This is now out of date; I am working on getting this PR up to speed with the new designs. |
Confusingly, Nheko (the only other native client) uses |
I recommend adding "fixes #192" in the PR description, that way github will link this PR as a potential closer for that issue. |
@DanilaFe I was just lazy and wanted an easy way to open a room in Nheko. Once this lands, we can probably just remove the open button for Nheko. :3 |
Fixes #192