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

[core] New UI API #763

Merged
merged 31 commits into from
Nov 9, 2023
Merged

[core] New UI API #763

Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
5eb80d8
Create basis of `UI` API
confused-Techie Oct 15, 2023
7332137
Compartmentalize all aspects of markdown handling to leave room for o…
confused-Techie Oct 18, 2023
c9c9f1a
Cleanup syntax highlighting callback funcs
confused-Techie Oct 18, 2023
e7c14be
Migrate `notifications` to new API
confused-Techie Oct 18, 2023
4c590f0
Remove accidentally included test file
confused-Techie Oct 18, 2023
0211bf5
Fix SyntaxHighlighting, and converting to dom, give `markdown-preview…
confused-Techie Oct 19, 2023
ccd65af
Handle local links in link resolving, implement all features into `ma…
confused-Techie Oct 19, 2023
e64e0a4
Include `markdown-preview` line break settings
confused-Techie Oct 19, 2023
06334cb
Migrate `deprecation-cop`
confused-Techie Oct 19, 2023
c20005b
Migrate `autocomplete-plus`
confused-Techie Oct 19, 2023
8f88f11
Run install against dep changes
confused-Techie Oct 19, 2023
d72cf65
Migrate `settings-view`
confused-Techie Oct 19, 2023
30e0793
Compact disable settings
confused-Techie Oct 21, 2023
7816086
Comment out in progress to test properly
confused-Techie Oct 26, 2023
65f2e55
Properly create `strict` disable mode
confused-Techie Oct 27, 2023
8328f9c
Modify one spec to different element, fix opts on API usage
confused-Techie Oct 27, 2023
2db34e2
Add `dompurify` only require deps once
confused-Techie Oct 27, 2023
cbaa3ad
Allow sanitizing options, ensure links don't double on `/`, don't mod…
confused-Techie Oct 27, 2023
dc7674b
resolve syntax error
confused-Techie Oct 27, 2023
2eaed49
Fix comma
confused-Techie Oct 27, 2023
fff477e
Don't modify base64 images, strip ending slash in all links, fix href…
confused-Techie Oct 27, 2023
45c0161
Expect HTML5 preferred `<br>` instead of `<br/>`
confused-Techie Oct 27, 2023
1ec01d1
Merge branch 'ui-api' of https://github.com/pulsar-edit/pulsar into u…
confused-Techie Oct 27, 2023
134fb45
Stop appending `/blob/master` to repo URLs
confused-Techie Oct 27, 2023
6da049f
Add JSDoc comments
confused-Techie Nov 6, 2023
fb006f6
Add some minor specs
confused-Techie Nov 6, 2023
011ddf9
Merge branch 'master' into ui-api
confused-Techie Nov 7, 2023
3378746
Move Atom link transformations out, fix tests
confused-Techie Nov 7, 2023
46c0b3a
Add newlines to expected specs
confused-Techie Nov 7, 2023
339f228
Match path translations
confused-Techie Nov 7, 2023
d7f7fed
Add missing path delimiter
confused-Techie Nov 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions spec/ui-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@ describe("Renders Markdown", () => {
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/HEAD/readme.md">Hello</a></p>\n');
});
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');
Copy link
Member

Choose a reason for hiding this comment

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

Erm is this right?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@DeeDeeG DeeDeeG Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, it's missing part of the path.
Lemme add the delimiter and see if specs are still passing

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@DeeDeeG DeeDeeG Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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

Copy link
Member

@DeeDeeG DeeDeeG Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.)

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it genuinely passed this time: https://github.com/pulsar-edit/pulsar/actions/runs/6807195453/job/18509642550#step:7:2047 👍

});
});

Expand Down
Loading