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.

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

Add SIP-23: JSX for Snap interfaces #136

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add SIP-23: JSX for Snap interfaces #136

wants to merge 10 commits into from

Conversation

Mrtenz
Copy link
Member

@Mrtenz Mrtenz commented Mar 22, 2024

This adds a SIP outlining JSX support for Snap interfaces.

Closes #133.

@Mrtenz Mrtenz changed the title Add "JSX for Snap interfaces" proposal Add SIP-23: JSX for Snap interfaces Mar 22, 2024
@Mrtenz Mrtenz marked this pull request as ready for review March 22, 2024 13:30
@Mrtenz Mrtenz requested review from Montoya, ziad-saab and a team as code owners March 22, 2024 13:30
SIPS/sip-23.md Outdated Show resolved Hide resolved
SIPS/sip-23.md Outdated Show resolved Hide resolved
SIPS/sip-23.md Outdated Show resolved Hide resolved
SIPS/sip-23.md Outdated Show resolved Hide resolved
Copy link
Contributor

@GuillaumeRx GuillaumeRx left a comment

Choose a reason for hiding this comment

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

LGTM

GuillaumeRx
GuillaumeRx previously approved these changes Mar 26, 2024

```typescript
type AddressProps = {
address: `0x${string}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not make this a CAIP address?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is based on the current API, and we have no way to properly show CAIP addresses in the extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay, the underlying components expect that type...it's just for the identicons. We should change components in the extension to allow for other account strings. Also I meant the account id portion of the caip address, not the whole string. I was thinking we were limiting it to ethereum because of validation reasons or something.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this change would be the time to swap to CAIP? 🤔

We could support the old format when not using JSX though. But even if we support CAIP addresses the underlying components don't yet, so we will need to validate that they are Ethereum-like addresses.

SIPS/sip-23.md Outdated Show resolved Hide resolved
SIPS/sip-23.md Outdated Show resolved Hide resolved

```typescript
type AddressProps = {
address: `0x${string}`;
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this change would be the time to swap to CAIP? 🤔

We could support the old format when not using JSX though. But even if we support CAIP addresses the underlying components don't yet, so we will need to validate that they are Ethereum-like addresses.

SIPS/sip-23.md Show resolved Hide resolved
SIPS/sip-23.md Outdated Show resolved Hide resolved
SIPS/sip-23.md Show resolved Hide resolved
SIPS/sip-23.md Outdated Show resolved Hide resolved
SIPS/sip-23.md Outdated Show resolved Hide resolved
SIPS/sip-23.md Outdated Show resolved Hide resolved

```typescript jsx
<Footer>
<Button onClick={handleCancel}>Cancel</Button>
Copy link
Member

Choose a reason for hiding this comment

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

We need to figure out how to handle this given that @eriknson want to keep the default Cancel button on all dialogs but allow two customizable footer buttons in home pages.

Not sure if it should be part of the spec? 🤔

Comment on lines +332 to +336
type DropdownProps = {
name: string;
value?: string | undefined;
children: OptionElement | OptionElement[];
};
Copy link
Contributor

Choose a reason for hiding this comment

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

missing onChange ?

type RowProps = {
label: string;
children: AddressElement | ImageElement | TextElement | ValueElement;
variant?: 'default' | 'warning' | 'error';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
variant?: 'default' | 'warning' | 'error';
variant?: 'default' | 'warning' | 'critical';

address, image, text, or value element.

In addition, the component accepts an optional `variant` prop which can be
`'default'`, `'warning'`, or `'error'`. The default value is `'default'`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`'default'`, `'warning'`, or `'error'`. The default value is `'default'`.
`'default'`, `'warning'`, or `'critical'`. The default value is `'default'`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create JSX SIP
4 participants