Skip to content

feat(candid): better UI for composite types#374

Merged
chenyan-dfinity merged 34 commits intomasterfrom
idl-ui
Mar 3, 2020
Merged

feat(candid): better UI for composite types#374
chenyan-dfinity merged 34 commits intomasterfrom
idl-ui

Conversation

@chenyan-dfinity
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity commented Feb 12, 2020 •
edited
Loading

It looks like this:
Screen Shot 2020-03-02 at 3 02 43 PM

@nomeata
Copy link
Contributor

nomeata commented Feb 18, 2020

Minor UX issue:
When I use a canister ID that does not exist or has no frontend included, then the error Error: An error happened while retrieving asset "candid.js". is only visible in the console, not on the page itself.

@chenyan-dfinity chenyan-dfinity marked this pull request as ready for review February 26, 2020 19:04
@chenyan-dfinity chenyan-dfinity requested a review from a team as a code owner February 26, 2020 19:04
eftychis
eftychis reviewed Mar 3, 2020
View reviewed changes
src/userlib/js/bootstrap/candid/candid.js
})().catch(err => {
left.className += ' error';
left.innerText = err.name + ': ' + err.message;
left.innerText = err.stack;
Copy link
Contributor

@eftychis eftychis Mar 3, 2020

Choose a reason for hiding this comment

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

there is no err body here?

Copy link
Contributor Author

@chenyan-dfinity chenyan-dfinity Mar 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

stack includes everything. The error message and the call stack.

hansl
hansl reviewed Mar 3, 2020
View reviewed changes
src/userlib/js/src/index.ts Outdated

import * as IDL from './idl';
export { IDL };
import * as UI from './idl-ui';
Copy link
Contributor

@hansl hansl Mar 3, 2020

Choose a reason for hiding this comment

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

This will break Node and TypeScript.

eftychis
eftychis reviewed Mar 3, 2020
View reviewed changes
src/userlib/js/bootstrap/candid/index.html
border: 1px solid #E5E5E5;
color: #545454;
font-family: sans-serif;
font-family: monospace;
Copy link
Contributor

@eftychis eftychis Mar 3, 2020

Choose a reason for hiding this comment

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

no idea about this whole css section from me.

cc @hansl if you can take a look.

eftychis
eftychis reviewed Mar 3, 2020
View reviewed changes
src/userlib/js/src/idl.ts
public visitType<T>(t: Type<T>, data: D): R {
throw new Error('Not implemented');
}
public visitPrimitive<T>(t: PrimitiveType<T>, data: D): R {
Copy link
Contributor

@eftychis eftychis Mar 3, 2020

Choose a reason for hiding this comment

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

this seems a bit too manual? Any way to generate?

Copy link
Contributor Author

@chenyan-dfinity chenyan-dfinity Mar 3, 2020

Choose a reason for hiding this comment

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

The hard work is done here. You don't need to implement all the visit functions, e.g. you can implement only the visitPrimitive function, because all the primitive types call visitPrimitive by default. See example from idl-ui.ts.

@hansl
Copy link
Contributor

hansl commented Mar 3, 2020

LGTM, we can get this in the release. You tested it manually?

@chenyan-dfinity
Copy link
Contributor Author

chenyan-dfinity commented Mar 3, 2020

Yep, works locally.

@chenyan-dfinity chenyan-dfinity merged commit 31fab2f into master Mar 3, 2020
@chenyan-dfinity chenyan-dfinity deleted the idl-ui branch March 3, 2020 20:18
dfinity-bot added a commit that referenced this pull request Mar 8, 2021
@dfinity-bot
build: niv common: update a1a1d2ad -> 43c86c9f
d2daf15 
## Changelog for common:
Branch: master
Commits: [dfinity-lab/common@a1a1d2ad...43c86c9f](https://github.com/dfinity-lab/common/compare/a1a1d2adcecd48b98a220d968ea8d477e242d01d...43c86c9f80a82577da4c35bf7c1ece491d71016d)

* [`43c86c9f`](https://github.com/dfinity-lab/common/commit/43c86c9f80a82577da4c35bf7c1ece491d71016d) darwin: Only build required jobs ([dfinity-lab/common⁠#374](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/374))
mergify bot pushed a commit that referenced this pull request Mar 8, 2021
@dfinity-bot
build: niv common: update a1a1d2ad -> 43c86c9f (#1478)
c0a565e 
## Changelog for common:
Branch: master
Commits: [dfinity-lab/common@a1a1d2ad...43c86c9f](https://github.com/dfinity-lab/common/compare/a1a1d2adcecd48b98a220d968ea8d477e242d01d...43c86c9f80a82577da4c35bf7c1ece491d71016d)

* [`43c86c9f`](https://github.com/dfinity-lab/common/commit/43c86c9f80a82577da4c35bf7c1ece491d71016d) darwin: Only build required jobs ([dfinity-lab/common⁠#374](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/374))
dfinity-bot added a commit that referenced this pull request Mar 10, 2021
@dfinity-bot
build: niv common: update 43c86c9f -> 22ad8d18
8dd7b98 
## Changelog for common:
Branch: master
Commits: [dfinity-lab/common@43c86c9f...22ad8d18](https://github.com/dfinity-lab/common/compare/43c86c9f80a82577da4c35bf7c1ece491d71016d...22ad8d18f6b20cedf6b4501754c151c7c7bc1bae)

* [`6242df59`](https://github.com/dfinity-lab/common/commit/6242df59f04fa680cc3627d5653bc6d20a03444d) [VER-612] Do not patch libunwind, as it makes _Unwind_RaiseException unsafe to call ([dfinity-lab/common⁠#376](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/376))
* [`9a6ce05f`](https://github.com/dfinity-lab/common/commit/9a6ce05fb33bacb8f9c278e6b4dbea26137d1f4f) Take LLVM 9 out of tree ([dfinity-lab/common⁠#378](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/378))
* [`22ad8d18`](https://github.com/dfinity-lab/common/commit/22ad8d18f6b20cedf6b4501754c151c7c7bc1bae) Revert "darwin: Only build required jobs ([dfinity-lab/common⁠#374](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/374))" ([dfinity-lab/common⁠#379](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/379))
mergify bot pushed a commit that referenced this pull request Mar 10, 2021
@dfinity-bot
build: niv common: update 43c86c9f -> 22ad8d18 (#1490)
97df09d 
## Changelog for common:
Branch: master
Commits: [dfinity-lab/common@43c86c9f...22ad8d18](https://github.com/dfinity-lab/common/compare/43c86c9f80a82577da4c35bf7c1ece491d71016d...22ad8d18f6b20cedf6b4501754c151c7c7bc1bae)

* [`6242df59`](https://github.com/dfinity-lab/common/commit/6242df59f04fa680cc3627d5653bc6d20a03444d) [VER-612] Do not patch libunwind, as it makes _Unwind_RaiseException unsafe to call ([dfinity-lab/common⁠#376](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/376))
* [`9a6ce05f`](https://github.com/dfinity-lab/common/commit/9a6ce05fb33bacb8f9c278e6b4dbea26137d1f4f) Take LLVM 9 out of tree ([dfinity-lab/common⁠#378](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/378))
* [`22ad8d18`](https://github.com/dfinity-lab/common/commit/22ad8d18f6b20cedf6b4501754c151c7c7bc1bae) Revert "darwin: Only build required jobs ([dfinity-lab/common⁠#374](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/374))" ([dfinity-lab/common⁠#379](http://r.duckduckgo.com/l/?uddg=https://github.com/dfinity-lab/common/issues/379))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@eftychis eftychis eftychis left review comments

@hansl hansl hansl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chenyan-dfinity @nomeata @hansl @eftychis