[core] New UI API
#763
[core] New UI API
#763
Conversation
|
This sounds cool. I'm not super familiar with the usage of markdown throughout the app, but this does sounds like it consolidates dependencies into one place, and makes for one source of truth for markdown, internally to the app and to whichever community packages that would opt-in to using it. I don't know if a different name for it would make sense, like Also it might be "illuminating" for reviewers to implement a usage of this API, maybe on a separate demonstration branch, but I don't think I would totally get it intuitively even if that were done. Not asking for that, but suggesting it might help people understand it if there are indeed reviewers forthcoming who are familiar with this stuff, which isn't a given. (Don't want to impose on your real time for a hypothetical reviewing-oriented benefit, though!) |
|
@DeeDeeG Thanks for taking a look here. But exactly, seems like a really cool and beneficial concept. But don't worry about want for an implementation example, the reason this is still in a draft is I'm hoping to migrate all core packages over to this new API prior to making the PR ready to review. As that'll ensure we can test it properly, as well as will give an implementation example, or even several. As for naming, I'd be totally fine changing it, or even be totally fine appending new UI type APIs to this same endpoint. To where it does more than just markdown handling, but that remains to be seen as even an option or idea, so if you'd rather change it, by all means we can, but also like you said, not the highest priority. |
…ify base64 links, make test results match universal GitHub links
… links to use `blob` URLs
Most browsers already strip self closing tags within void elements. DOMPurify itself also strips this character.
1. `master` isn't always true. We can use `HEAD` now and GitHub will automatically redirect to the main branch. 2. We already have the logic inside our markdown to correct links to use `blob` or `raw` depending on if directing to a link or image, and use `HEAD` like mentioned above
|
Alright it's taken some time, but this one is finally ready for a review! In this PR I have successfully implemented the Markdown features of the new Features Supported
Packages Migrated
With that writeup done, it's important to note that this feature set covers everything that was original used across all core packages and puts it all together in a single API, that now any core package or community package can utilize. So while this is a bigger one, this is now ready for review, and once successfully reviewed we can merge #774 into this branch then bring everything into |
There was a problem hiding this comment.
First thing that I have to say: INCREDIBLE JOB!
I am really, really exited to use this on my own plug-in (that currently have to render markdown by itself, causing a whole lot of weird issues and different behavior than what's expected).
Now, I just added 3 requests for changes - they all relate to documentation. Because we're adding a new API, and we still don't have the API Docs anywhere, it's a good idea to add some JSDocs to avoid us forgetting about the new API we added.
With the JSDoc in place, I say: merge it, and let's go! :)
|
@mauricioszabo I really appreciate your review! |
There was a problem hiding this comment.
I'm a "lite-approve" on this, as multiple people seem to really want it. I support it mainly for that reason.
Feels a bit beyond my level to properly review every bit of the diff, but I'd be willing to lend a hand troubleshooting it if anything were to go wrong post-merge.
spec/ui-spec.js
Outdated
| }); | ||
| it("resolves incomplete root links", () => { | ||
| expect(atom.ui.markdown.render( | ||
| "[Hello](/readme.md)", | ||
| { rootDomain: "https://github.com/pulsar-edit/pulsar" } | ||
| )).toBe('<p><a href="https://github.com/pulsar-edit/pulsar/readme.md">Hello</a></p>\n'); | ||
| )).toBe('<p><a href="https://github.com/pulsar-edit/pulsar/blob/HEADreadme.md">Hello</a></p>\n'); |
There was a problem hiding this comment.
Fantastic catch, no, no it is not. Lemme fix that, but makes me wonder why the CI passed on it, since it shouldn't have
There was a problem hiding this comment.
Shouldn't it be HEAD/readme.md with the slash, not HEADreadme.md?
EDIT: I'm too slow typing this, didn't see you had replied before I refreshed the page, heh.
There was a problem hiding this comment.
Exactly, it's missing part of the path.
Lemme add the delimiter and see if specs are still passing
There was a problem hiding this comment.
I've added the delimiter, and checked that the code should be making this change, so not sure why we got a reported success if that was not the case. But lets let the specs run and see if it was a fluke
There was a problem hiding this comment.
Would you believe the spec failed on exactly this test? EDIT: to be clear, the passing CI run had a spec failure in it, actually.
https://github.com/pulsar-edit/pulsar/actions/runs/6780796150/job/18430013470#step:7:2435
Looks like our workflow (our test runner script, specifically?) kept going and ended up with an apparently erroneous "clean" exit code from some stuff after the failure? Hmmm.
There was a problem hiding this comment.
@DeeDeeG Even crazier, would you believe how it passed on the run immediately after this?
Finished in 0.039 seconds
0 tests, 0 assertions, 0 failures, 2283 skipped
There was a problem hiding this comment.
I think the test runner script/run-tests.js was trying to filter to run again, but only for the failing tests, but maybe it filtered too hard and didn't leave any tests in at all? (A loose guess.)
There was a problem hiding this comment.
Looks like it genuinely passed this time: https://github.com/pulsar-edit/pulsar/actions/runs/6807195453/job/18509642550#step:7:2047 👍
|
Thanks for the review both to @mauricioszabo and @DeeDeeG, and thanks for offering to help post merge @DeeDeeG! Mauricio has confirmed on Discord the changes look good and we are a go for merge. Lets get this one into core in time for the next release! PS. Also big thanks to DeeDee for catching that last minute bug! |
As previously discussed on Discord, there seems to be a good need for the creation of a
UIAPI, that at least at first, can make it's focus on rendering Markdown content.Within Pulsar there are many packages that have to render Markdown content, each of which containing disparate versions of the same dependencies over and over. Where when updating one of these, many packages have to be modified, and there have to be many checks for changes in behavior.
Additionally, over in
pulsar-edit/package-frontendthere have been many changes made to how we render Markdown content from package readmes, that for a while I've wanted to bring over tosettings-view, such as converting links to the Atom docs to use WebArchive, or converting links to other packages published to Pulsar into links using the new Pulsar domain, etc. But these changes have never been easily possibly becausesettings-viewwas usingmarked, meanwhilepackage-frontendusesmarkdown-it.But with all of this said, onto this proposed solution:
This PR creates a new core API endpoint of
atom.uithat exposes several functions that can aid in packages parsing Markdown content. With the intention that we can remove all markdown handling fromsettings-view,notifications,markdown-preview,deprecation-cop,autocomplete-plus, etc. And we instead move all of these packages to utilize this new endpoint.This change will then of course be available to community package's as well for these improvements.
The benefit of a change like this can potentially be far reaching, while concretely gives us real world benefits today: