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

Moving static data out of Element client file #273

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

thibaultamartin
Copy link
Contributor

No description provided.

@thibaultamartin thibaultamartin marked this pull request as draft June 9, 2022 10:47
this.data = data;
}

get id() { return this.data.id; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it okay to snake case the project name as an id?

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what you mean here. this line seems fine to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bwindels right now id is a field that only exist for matrix.to

At the moment for Element, the id is element.io.
Would it be ok to use element instead? I would derive the id from the project name (since two projects can't have the same name anyway)

src/open/clients/Element.js Outdated Show resolved Hide resolved
src/open/clients/Client.js Show resolved Hide resolved
this.data = data;
}

get id() { return this.data.id; }
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what you mean here. this line seems fine to me.

src/open/clients/Client.js Show resolved Hide resolved
src/open/clients/Element.js Outdated Show resolved Hide resolved
"applestorelink": new AppleStoreLink('vector', 'id1083446067'),
"playstorelink": new PlayStoreLink('im.vector.app'),
"fdroidlink": new FDroidLink('im.vector.app'),
"flathublink": new FlathubLink('im.riot.Riot'),
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 these links something like this? You can wrap these strings into their *Link objects in Client.getInstallLinks, that way it's closer to json and people don't have to deal with knowing about these classes:

"installLinks": {
  "applestore": ["vector", "id1083446067"],
  "playstore": "im.vector.app",
  "fdroid": "im.vector.app",
  "flathub": "im.riot.Riot",
  "website": "https://element.io/get-started"
}

Copy link
Member

Choose a reason for hiding this comment

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

I think at that point "installLinks" becomes misleading, because im.vector.app isn't a link

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like as much of the content as possible to be generated by squiddy (to make sure it will be kept up to date).

The file format for squiddy is not completely dry yet, but for clients it would look like

[[clients]]
title = "Element Web"
description = "Element is a glossy web client with an emphasis on performance and usability"
maturity = "Stable"
(more fields…)
appstore_details.org = "vector"
appstore_details.app_id = "id1083446067"
playstore_app_id = "im.vector.app"
fdroid_app_id = "im.vector.app"
flathub_app_id = "im.vector.app"
otherinstall_link = ["https://element.io/get-started"]
full_description = """
Longer multi-line
description of the client
that can [include links](#) and `markdown in general`
"""
[[clients.authors]]
name = "Element"
[[clients.authors]]
name = "Kat"
matrix_id = "@kittykat:matrix.org"
[[clients.authors]]
name = "Danielle Kirkwood"
matrix_id = "@daniellekirkwood:one.ems.host"

Under those circumstances, it's not a huge problem to use the classes. Is it acceptable for you?

src/open/clients/ElementData.js Show resolved Hide resolved
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.

3 participants