-
Notifications
You must be signed in to change notification settings - Fork 3
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
Go to newly created Lexeme #94
Conversation
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.
All in all, I like this approach of injecting a "WikiRouter" to handle navigation out of the app. I added a couple of comments about things that are not so obvious at first glance, and might require some additional clarification.
private readonly getUrl: MwUtilGetUrl; | ||
|
||
public constructor( | ||
getUrl: MwUtilGetUrl, |
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.
While I understand why this is injected here, it is not self-evident why we would need to inject a service that gets / generates a URL for us (and why this is better than just having a base URL as a config instead and constructing the URL ourselves). Therefore, I think it would be beneficial to anyone reading this code in the future if a comment was added to document this dependency.
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.
That is a good point! Though I feel I'm not at my most eloquent today. I added a first comment, but I'd be happy about ideas for how to improve it
I think this is now also ready for review. Happy to hear any suggestions for improvement :) |
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 locally, seems to work fine. One optional comment.
Also remove a stray import. No idea how that got *there*.
Co-authored-by: Itamar Givon <itamar.givon.dev@gmail.com>
We don't need it for now. We can partially readd it when we need it for getPageUrl.
(rebased on top of main) |
The plugin code is itself not related to MediaWiki, so it should live with the agnostic interface. Especially, since it only mentions that interface but not any specific implementation.
Todo:
Bug: T304397