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

chore(#1105): add icons instagram, message, x, youtube #1115

Conversation

BrianGilbert
Copy link

Checklist before requesting a review

  • I have formatted the subject to include ticket number as [CIVIC-123] Verb in past tense with dot at the end.
  • I have added a link to the JIRA ticket
  • I have provided information in Changed section about WHY something was done if this was not a normal implementation
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have run new and existing relevant tests locally with my changes, and they passed
  • I have provided screenshots, where applicable

Changed

Screenshots

@AlexSkrypnyk
Copy link
Contributor

AlexSkrypnyk commented Oct 2, 2023

@BrianGilbert
thank you for providing a PR

We only provide assets for components that are currently used in the default implementation of the CivicTheme. What this means is that we can accept the changes to the assets used in the existing components (like changing Twitter to X), but we cannot accept new assets unless they are used in new components.

The reasoning behind this is simple: not everyone want/needs all these assets in their build. CivicTheme used to ship 1200+ icons as SVGs, but only ~20 were used. We had complaints about why someone need to deal with this high number of icons which are not used in the build at all, so we removed them.

--

Regarding the change of Twitter to X - we actually need to change the whole Social Links component and update the asset. Only adding x.svg as in this PR will not work for us. It has to be a full component update.

@BrianGilbert
Copy link
Author

@AlexSkrypnyk social links story updated to include additional icons, twitter icon removed

@AlexSkrypnyk
Copy link
Contributor

@BrianGilbert
Thank you for updating the PR. It looks good!

It will have to wait until the next week at least to be properly tested and merged.

@AlexSkrypnyk AlexSkrypnyk self-requested a review October 2, 2023 08:02
@AlexSkrypnyk AlexSkrypnyk added State: DO NOT MERGE Do not merge this pull request State: BLOCKED The work on the issue is blocked labels Oct 2, 2023
@AlexSkrypnyk
Copy link
Contributor

Note to self: need to extend this PR with changes to Drupal theme, run CI and merge.

@AlexSkrypnyk AlexSkrypnyk changed the base branch from develop to feature/update-social-icons November 9, 2023 01:35
@AlexSkrypnyk
Copy link
Contributor

closed in favour of #1144

@BrianGilbert
thank you for working on this. The credits will be preserved (and if you are using the same email as in D.O. - the credit will be visible there).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
State: BLOCKED The work on the issue is blocked State: CONFLICT State: DO NOT MERGE Do not merge this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants