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

twitter to x icon #13247

Merged
merged 5 commits into from
Oct 24, 2023
Merged

Conversation

sarthak-kumar-shailendra
Copy link
Contributor

@sarthak-kumar-shailendra sarthak-kumar-shailendra commented Oct 14, 2023

Done

Changed the svg of twitter icon to x

QA

  • Check out this feature branch
  • Run the site using the command ./run serve or dotrun
  • View the site locally in your web browser at: https://ubuntu-com-13247.demos.haus/
    • Be sure to test on mobile, tablet and desktop screen sizes
  • [List additional steps to QA the new features or prove the bug has been resolved]

Issue / Card

Fixes #13246 , https://warthogs.atlassian.net/browse/WD-6905

Screenshots

Screenshot 2023-10-14 at 4 29 01 PM

Help

QA steps - Commit guidelines

@webteam-app
Copy link

webteam-app commented Oct 14, 2023

Demo running at https://ubuntu-com-13247.demos.haus

@sarthak-kumar-shailendra
Copy link
Contributor Author

@mtruj013 can you pls check this?

@mtruj013 mtruj013 self-assigned this Oct 16, 2023
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #13247 (8039555) into main (621f09d) will not change coverage.
Report is 23 commits behind head on main.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main   #13247   +/-   ##
=======================================
  Coverage   75.31%   75.31%           
=======================================
  Files         106      106           
  Lines        2840     2840           
  Branches      929      929           
=======================================
  Hits         2139     2139           
  Misses        679      679           
  Partials       22       22           

@mtruj013
Copy link
Contributor

Thanks for your contribution @sarthak-kumar-shailendra! This needs a design review, I'll go ahead and review the code once that has been done as there may be some changes requested. cc: @lyubomir-popov

@lyubomir-popov
Copy link
Contributor

Hi can you please define the height in rems not pixels?
On smaller screens, everything looks fine:
image
But on screens larger than 1680px, where we increase the value of 1rem, things get misaligned for two of the icons:
image

Bonus points for fixing the 4th icon as well :)

@sarthak-kumar-shailendra
Copy link
Contributor Author

@lyubomir-popov Have converted pixels to rems and fixed the fourth icon as well. Can you check again?

@lyubomir-popov
Copy link
Contributor

hi, thanks for doing this - but there's still something keeping it smaller @sarthak-kumar-shailendra:

image

maybe it is the 32px height on the svg itself? Can you please compare how the two in the middle (the bigger ones) are built?

@sarthak-kumar-shailendra
Copy link
Contributor Author

This is how it's looking for screen having width 1698px in local. Are we checking the latest changes?

Screenshot 2023-10-17 at 4 05 28 PM

@lyubomir-popov
Copy link
Contributor

lyubomir-popov commented Oct 17, 2023

I believe I'm looking at the latest demo - just opened it in a different browser with cleared cache, they still seem different heights:
image

(never mind the dark background, that's a feature of the brave browser)

@sarthak-kumar-shailendra
Copy link
Contributor Author

is my last commit published in the latest demo?
I am asking because even I was getting 2 larger icons in local when height was mentioned in px (for larger screens) but after changing it to rem there's no spacing issue for both smaller and larger screens.
@lyubomir-popov @mtruj013

@mtruj013
Copy link
Contributor

@lyubomir-popov @sarthak-kumar-shailendra looks like new pushes aren't triggering demo builds if they come from external contributors. I've manually triggered a build, it should be up to date now (same link)

@sarthak-kumar-shailendra
Copy link
Contributor Author

@lyubomir-popov can you check now?

@lyubomir-popov
Copy link
Contributor

lyubomir-popov commented Oct 18, 2023

Thanks @sarthak-kumar-shailendra the sizing is now correct, but I also noticed the X letter is transparent, and shows the background underneath. Can you please use the same colour as other icons in the set (light gray, solid). This is to ensure readability even when a user is using plugins/browser features that alter page backgrounds, as demonstrated in the brave browser screenshot above.

@sarthak-kumar-shailendra
Copy link
Contributor Author

@lyubomir-popov did you mean like this?
Screenshot 2023-10-20 at 4 45 30 PM

@lyubomir-popov
Copy link
Contributor

@sarthak-kumar-shailendra yes but please use the same colour as other icons in this set: #e5e5e5

@sarthak-kumar-shailendra
Copy link
Contributor Author

@lyubomir-popov have pushed the changes, will need to trigger a new build manually

@mtruj013
Copy link
Contributor

@sarthak-kumar-shailendra @lyubomir-popov demo should be up to date

Copy link
Contributor

@mtruj013 mtruj013 left a comment

Choose a reason for hiding this comment

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

@mtruj013 mtruj013 merged commit 771847f into canonical:main Oct 24, 2023
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request to Update Twitter Icon to X
5 participants