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

Add new Servo Logo #25749

Merged
merged 9 commits into from Feb 25, 2020
Merged

Add new Servo Logo #25749

merged 9 commits into from Feb 25, 2020

Conversation

@malqinneh
Copy link
Contributor

malqinneh commented Feb 12, 2020

  • Added servo-64.png (replacing servo64.png @ 64px)
  • Added servo-100.png (replacing doge-tiny.png @ 100px)
  • Added servo-500.png
  • Added servo-500-icon.ico
  • Added servo-1000.png (replacing servo.png @ 1000px)
  • Added servo.svg (replacing servo.svg)

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because they add new logo assets
malqinneh added 3 commits Feb 12, 2020
* Added servo-64.png (replacing servo64.png @ 64px)
* Added servo-100.png (replacing doge-tiny.png @ 100px)
* Added servo-500.png
* Added servo-500_icon.ico
* Added servo-1000.png (replacing servo.png @ 500px)
* Added servo.svg (replacing servo.svg)
* Added servo-500-icon.ico
@highfive
Copy link

highfive commented Feb 12, 2020

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @nox (or someone else) soon.

@paulrouget
Copy link
Contributor

paulrouget commented Feb 13, 2020

Can you also update support/android/apk/servoapp/src/main/res/mipmap/servo.png ?

malqinneh added 4 commits Feb 13, 2020
* Added servo-1000.png (replacing servo.png @ 1024px)
* Added servo-1024.png (replacing servo.png @ 1024px)
* Added servo-1024.png
@malqinneh
Copy link
Contributor Author

malqinneh commented Feb 13, 2020

Can you also update support/android/apk/servoapp/src/main/res/mipmap/servo.png ?

Absolutely

@paulrouget
Copy link
Contributor

paulrouget commented Feb 14, 2020

@jdm do we want to land that now?

@nox nox assigned jdm and unassigned nox Feb 14, 2020
@jdm
Copy link
Member

jdm commented Feb 24, 2020

@bors-servo r+
Thanks @malqinneh!

@bors-servo
Copy link
Contributor

bors-servo commented Feb 24, 2020

📌 Commit 5314625 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Feb 24, 2020

Testing commit 5314625 with merge 06678e7...

bors-servo added a commit that referenced this pull request Feb 24, 2020
Add new Servo Logo

<!-- Please describe your changes on the following line: -->

* Added servo-64.png (replacing servo64.png @ 64px)
* Added servo-100.png (replacing doge-tiny.png @ 100px)
* Added servo-500.png
* Added servo-500-icon.ico
* Added servo-1000.png (replacing servo.png @ 1000px)
* Added servo.svg (replacing servo.svg)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because they add new logo assets

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 24, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented Feb 24, 2020

> /repo/support/android/apk/servoapp/src/main/res/mipmap/servo-1024.png: Error: '-' is not a valid file-based resource name character: File-based resource names must contain only lowercase a-z, 0-9, or underscore
@malqinneh
Copy link
Contributor Author

malqinneh commented Feb 24, 2020

> /repo/support/android/apk/servoapp/src/main/res/mipmap/servo-1024.png: Error: '-' is not a valid file-based resource name character: File-based resource names must contain only lowercase a-z, 0-9, or underscore

Is this a matter of replacing - (dash) with _ (underscore) in the filename?

@jdm
Copy link
Member

jdm commented Feb 24, 2020

I expect that would work, but we should really overwrite the existing servo.png instead so that the new image gets used automatically.

@malqinneh
Copy link
Contributor Author

malqinneh commented Feb 24, 2020

I expect that would work, but we should really overwrite the existing servo.png instead so that the new image gets used automatically.

Makes sense but tried to avoid overwriting original PNG file(s) to keep them safe/available in case they are needed in the future. I suppose one can always go back in commit history for them.

@jdm
Copy link
Member

jdm commented Feb 24, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Feb 24, 2020

📌 Commit 4ea7da8 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Feb 24, 2020

Testing commit 4ea7da8 with merge e3a5f36...

@bors-servo
Copy link
Contributor

bors-servo commented Feb 25, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing e3a5f36 to master...

@bors-servo bors-servo merged commit e3a5f36 into servo:master Feb 25, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@atouchet
Copy link
Contributor

atouchet commented Feb 25, 2020

This doesn't appear to have updated the logo on Windows. Servo.ico, servo.png, and servo64.png are still using the old logo.

@malqinneh
Copy link
Contributor Author

malqinneh commented Feb 25, 2020

This doesn't appear to have updated the logo on Windows. Servo.ico, servo.png, and servo64.png are still using the old logo.

Probably due to file name(s). In order for the new assets to simply replace the old ones, they need to have the same name as the old assets. Alternatively, we can update the line of code that references old asset names to the new names.

@atouchet
Copy link
Contributor

atouchet commented Feb 25, 2020

@malqinneh I'm not sure what the best approach would be but I took a quick look and it appears the relevant references are here:

res.set_icon("../../resources/Servo.ico");

icon_path = path.join(self.get_top_dir(), "resources", "servo.png")

let icon_bytes = include_bytes!("../../resources/servo64.png");

@malqinneh
Copy link
Contributor Author

malqinneh commented Feb 25, 2020

@atouchet happy to update those items. I need to create a new fork of the Servo branch since the last one I started was merged, is that correct?

@atouchet
Copy link
Contributor

atouchet commented Feb 25, 2020

That should work. Taking another look at the files in /resources I suspect that servo.icns may be another file that needs updating but it appears to be an Apple image format and I don't have a Mac.

Edit: The reference to servo.icns is located at:

<string>servo.icns</string>

@malqinneh
Copy link
Contributor Author

malqinneh commented Feb 25, 2020

@atouchet created a new pull request. Let me know if we missed anything. Thank you very much for your help and patience 👍

@atouchet
Copy link
Contributor

atouchet commented Feb 25, 2020

Thanks! I will take a look at the PR.

alrra referenced this pull request in alrra/browser-logos Jun 14, 2020
* Move old `Servo` logo into the archive.
* Add new `Servo` logo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.