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(ui): Match account metadata icon names with trailing colons and add/update more icons #1693

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

Conversation

ZMYaro
Copy link

@ZMYaro ZMYaro commented Feb 10, 2023

This also addresses the email half of #1683.

@stackblitz
Copy link

stackblitz bot commented Feb 10, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Feb 10, 2023

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit aa9951b
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/6413946090f5e60008e9c3ab

@netlify
Copy link

netlify bot commented Feb 10, 2023

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit aa9951b
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/64139460a3418c00072362e2
😎 Deploy Preview https://deploy-preview-1693--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ZMYaro
Copy link
Author

ZMYaro commented Feb 10, 2023

Specifically, I added/altered Birth, Birthday, BuyMeACoffee, 'Buy Me A Coffee', Chat, Email, E-mail, Facebook, Home, 'Home Page', Homepage, IRC, Joined, KoFi, Ko-fi, Meatspace, Nintendo Switch, Skype, Slack, Snapchat, Substack, and Tumblr.

This also fixes it so a field called Email:, for example, still gets the icon for Email.

(Edits: I added more; I think this is all of them. I also rebased this so putting quotes around all icons is a separate commit so it is clear in the diff where I added icons.)

@ZMYaro ZMYaro changed the title Match account metadata icon names with trailing colons and add/update more icons feat(ui): Match account metadata icon names with trailing colons and add/update more icons Feb 13, 2023
@ZMYaro ZMYaro force-pushed the more-metadata-icons branch from b8e1696 to 0555517 Compare February 14, 2023 17:44
@Shinigami92 Shinigami92 added the c: ui Enhancing UI label Feb 25, 2023
@ZMYaro ZMYaro force-pushed the more-metadata-icons branch from 92f8e7d to 00f7829 Compare March 13, 2023 22:16
@patak-dev
Copy link
Member

We need to implement the matching separated from the list (maybe you could add an alias field or a new alias map?). The issue with the PR right now is that when rendering the full list to pick from, you get several duplicates:
image

@ZMYaro
Copy link
Author

ZMYaro commented Mar 13, 2023

We need to implement the matching separated from the list (maybe you could add an alias field or a new alias map?). The issue with the PR right now is that when rendering the full list to pick from, you get several duplicates

@patak-dev I didn't realize that was something you wanted addressed in this PR since the duplicate key icons were already present. I can add an alias map later.

@ZMYaro
Copy link
Author

ZMYaro commented Mar 14, 2023

@patak-dev Looking into this now, even after constructing an alias map, I am not sure how you want to decide which text label is the default for each icon. For instance, I am not sure how to decide whether the key icon should insert the “GPG”, “Keyoxide”, “PGP”, or “OpenPGP” label by default.

@patak-dev
Copy link
Member

@ZMYaro ah, I didn't realize that PGP and Language were both already duplicated. I think the issue with this PR is that it is a lot more evident after it.

About what Label to use, we could select the most common one as the default (or any of them if all are similar in use). This is only to avoid duplicates in the picker, so I think it is ok to pick one of them. If the user writes the label by hand, the icon would still be properly assigned.

@ZMYaro
Copy link
Author

ZMYaro commented Mar 14, 2023

@patak-dev I am thinking then having the alias list isn't the best solution for that menu, and it would be better to have a list of featured labels specifically for that component? Because I agree we (for example) don't need both “Home Page” and “Homepage”, but it does makes sense to keep both “GPG” and “Keyoxide” available.

@patak-dev
Copy link
Member

In the case of “GPG” and “Keyoxide”, they better have different icons if possible, no? Not an issue if some of them end up duplicated though for now. Just that we can't have Ko-Fi and KoFi

@ZMYaro
Copy link
Author

ZMYaro commented Mar 14, 2023

In the case of “GPG” and “Keyoxide”, they better have different icons if possible, no?

@patak-dev Right now, they have the same icon (you can see right now in Elk stable). If we picked one label for each icon, we would have to pick one and rule out all the others. That was why I suggested it might work better to have a list of labels to suggest in the SettingsProfileMetadata menu and keep the existing map of labels to icons.

Edit: I pushed an implementation of that; let me know what you think.

@ZMYaro ZMYaro force-pushed the more-metadata-icons branch 3 times, most recently from 473cd7d to 04516f2 Compare March 16, 2023 08:44
@ZMYaro ZMYaro force-pushed the more-metadata-icons branch from 04516f2 to aa9951b Compare March 16, 2023 22:12
@patak-dev patak-dev linked an issue Apr 27, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: ui Enhancing UI
Projects
Status: Awaiting Review
Development

Successfully merging this pull request may close these issues.

Duplicate features profile link icons
3 participants