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

feat: Add Wallet Assets #22

Merged
merged 23 commits into from
Dec 15, 2023
Merged

Conversation

DavideSegullo
Copy link
Contributor

It would be useful to have the assets associated with the wallets, for example, their logo, as is done in chain-registry

@DavideSegullo DavideSegullo marked this pull request as ready for review December 11, 2023 11:09
@JeremyParish69
Copy link
Collaborator

JeremyParish69 commented Dec 11, 2023

This is fantastic work. My suggestion would be to remove logo_URIs, and only keep images

@DavideSegullo
Copy link
Contributor Author

This is fantastic work. My suggestion would be to remove logo_URIs, and only keep images

Yes it is a very good idea, we can compact it into the images field, how about if we also add an additional information to indicate the type of image though? For example "logo", "banner", "logotype" and "logomark", what do you think?

@JeremyParish69
Copy link
Collaborator

Yes it is a very good idea, we can compact it into the images field, how about if we also add an additional information to indicate the type of image though? For example "logo", "banner", "logotype" and "logomark", what do you think?

Yes, I like this. But there are so many different ways a logo can be presented, so I'd like to define what each of these mean.

E.g., I have a CMS that allows for 6 different logo images for each project: 3 color varieties (light mode, dark mode, and all white) of logo_asset (which, I think is same as what you call logomark), and 3 color varieties of logotype (which is the logo asset + text).
Also, whenever there are multiple variants of the logotype, I opt for the one that has the text to the right of the logo asset, and omit the one with text below the logo asset. If allowing for both varieties of a single logo, we should allow for a way to specify which is which.

@DavideSegullo
Copy link
Contributor Author

Yes it is a very good idea, we can compact it into the images field, how about if we also add an additional information to indicate the type of image though? For example "logo", "banner", "logotype" and "logomark", what do you think?

Yes, I like this. But there are so many different ways a logo can be presented, so I'd like to define what each of these mean.

E.g., I have a CMS that allows for 6 different logo images for each project: 3 color varieties (light mode, dark mode, and all white) of logo_asset (which, I think is same as what you call logomark), and 3 color varieties of logotype (which is the logo asset + text). Also, whenever there are multiple variants of the logotype, I opt for the one that has the text to the right of the logo asset, and omit the one with text below the logo asset. If allowing for both varieties of a single logo, we should allow for a way to specify which is which.

Yes, I agree, about the image's categories:

  1. Logo, I mean logo "icon" + text
  2. Logo type, I mean only text
  3. Logo mark, I mean only logo "icon"

If we wanna handle text direction, we can add a property "direction", that can be helpful for accessibility if someone need to use it for rtl

@JeremyParish69
Copy link
Collaborator

Yes, I agree, about the image's categories:

  1. Logo, I mean logo "icon" + text
  2. Logo type, I mean only text
  3. Logo mark, I mean only logo "icon"

If we wanna handle text direction, we can add a property "direction", that can be helpful for accessibility if someone need to use it for rtl

I see. My logotype definition was icon + text, so I hope I don't cause confusion there. I generally like your definitions.

On direction, all I meant was where the text is positioned relative to the icon(--not how it's read; ltr vs rtl).
image

@JeremyParish69
Copy link
Collaborator

could have something like this within the schema within an image object? I think we'll be able to make use of the same definitions for the Chain Registry.

"logo_layout": {
  "type": "string",
  "enum": [
    "logo",
    "logomark",
    "logotype"
  ],
  "description": "logomark == icon only; logotype == text only; logo == icon + text."
}

@DavideSegullo
Copy link
Contributor Author

"logo_layout": {
"type": "string",
"enum": [
"logo",
"logomark",
"logotype"
],
"description": "logomark == icon only; logotype == text only; logo == icon + text."
}

Yes, we add the following one inside the 'images' array:

{
  "layout": {
    "type": "string",
    "enum": [
      "logo",
      "logomark",
      "logotype"
    ],
    "description": "logomark == icon only; logotype == text only; logo == icon + text."
  },
  "text_position": {
    "type": "string",
    "enum": [
      "top",
      "bottom",
      "left",
      "right"
    ],
    "description": "Indicates in which position the text is placed, in case the layout is 'icon' type, it's required only in this case."
  },
}

I only changed the field name from "logo_layout" to "layout", just to be generic, right now we're using it for logo but maybe in the future we wanna use it for other use cases.

Just to avoid confusion (maybe a 'direction' field can be tricky), we can create a "text_position" field, that can be use for "logo" layout and it's required only in this use case.

@JeremyParish69
Copy link
Collaborator

Yes, we add the following one inside the 'images' array:

{
  "layout": {
    "type": "string",
    "enum": [
      "logo",
      "logomark",
      "logotype"
    ],
    "description": "logomark == icon only; logotype == text only; logo == icon + text."
  },
  "text_position": {
    "type": "string",
    "enum": [
      "top",
      "bottom",
      "left",
      "right"
    ],
    "description": "Indicates in which position the text is placed, in case the layout is 'icon' type, it's required only in this case."
  },
}

I only changed the field name from "logo_layout" to "layout", just to be generic, right now we're using it for logo but maybe in the future we wanna use it for other use cases.

Just to avoid confusion (maybe a 'direction' field can be tricky), we can create a "text_position" field, that can be use for "logo" layout and it's required only in this use case.

I think the text_position is a bit overkill, but I also really like. I'm happy with what you've suggested and would like to implement the same into the Chain Registry to help implement this as a standard.

@DavideSegullo
Copy link
Contributor Author

Yes, we add the following one inside the 'images' array:

{
  "layout": {
    "type": "string",
    "enum": [
      "logo",
      "logomark",
      "logotype"
    ],
    "description": "logomark == icon only; logotype == text only; logo == icon + text."
  },
  "text_position": {
    "type": "string",
    "enum": [
      "top",
      "bottom",
      "left",
      "right"
    ],
    "description": "Indicates in which position the text is placed, in case the layout is 'icon' type, it's required only in this case."
  },
}

I only changed the field name from "logo_layout" to "layout", just to be generic, right now we're using it for logo but maybe in the future we wanna use it for other use cases.
Just to avoid confusion (maybe a 'direction' field can be tricky), we can create a "text_position" field, that can be use for "logo" layout and it's required only in this use case.

I think the text_position is a bit overkill, but I also really like. I'm happy with what you've suggested and would like to implement the same into the Chain Registry to help implement this as a standard.

Thank you! I'll update the schema

@DavideSegullo
Copy link
Contributor Author

@JeremyParish69 Done

@JeremyParish69 JeremyParish69 merged commit 789a8e1 into cosmos:main Dec 15, 2023
1 check passed
@DavideSegullo DavideSegullo deleted the nabla/feat-assets branch December 15, 2023 20:40
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.

2 participants