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

Missing discord and GitHub links under the package description #885

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

Cyanideankst
Copy link
Contributor

@Cyanideankst Cyanideankst requested a review from Oksamies November 6, 2023 03:03
Copy link
Contributor

@anttimaki anttimaki left a comment

Choose a reason for hiding this comment

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

I don't mark these as blockers for the reasons mentioned below, but I think you and @Oksamies should be aware of these.

  1. Generally speaking, you shouldn't just add stuff to dapper and dapper-fake without also updating dapper-ts (and other implementations of Dapper, if we'd have others). Furthermore you shouldn't add stuff to dapper-ts without making sure the changes don't require changes to backend too. In this particular case it doesn't matter because the method is not yet implemented in dapper-ts, so nothing breaks.
  2. Generally speaking, you shouldn't add fields that can be undefined in dapper. Backend will return null (or possibly empty string) if a value is not available. In this particular case it doesn't matter since I will rethink that whole interface when I get to it.
  3. Package doesn't have a Discord link. Community might have one. Haven't gotten this far in Dapper implementations so I haven't really though which method should return this information. It's possible we'll fetch this information with getCommunityDetails instead. Again, doesn't matter too much in this case since I'll do some changes anyway when I get that far.
  4. For that matter, package doesn't have a GitHub link either, it has a "website link". So while linking to GitHub seems to be the standard way, the link might lead to somewhere else as well, so naming the field "gitHubLink" is wrong, and we shouldn't force the ButtonLabel/ButtonIcon either. Again, I can handle the Dapper side of things, but please solve the UI issue with gkurck.

TL;DR: perhaps Dapper-related changes should be coordinated with me, at least for the time being?

@MythicManiac MythicManiac merged commit 3d16c70 into master Nov 6, 2023
17 checks passed
@MythicManiac MythicManiac deleted the cyberstorm-package-missing-link branch November 6, 2023 12:23
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