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

v1.12.0 #46

Merged
merged 4 commits into from
Apr 5, 2024
Merged

v1.12.0 #46

merged 4 commits into from
Apr 5, 2024

Conversation

gabrielluong
Copy link
Collaborator

@gabrielluong gabrielluong commented Apr 3, 2024

Reformat android xml files and include the necessary tools:ignore

@jsimplicio
Copy link
Member

Hey gl,

Are the tools:ignore changes something we accidentally forgot or stripped away recently (we've implemented a new workflow so we're curious if it affected the format)? I know we're supposed to add tools:ignore="VectorPath" to complex paths but saw that your change had some different values assigned to this too (e.g. tools:ignore="VectorPath,VectorRaster"). Curious if there's something new we should be aware of.

Also curious if there's a preference for the existing order of fillColor and pathData.

Thanks!

@gabrielluong
Copy link
Collaborator Author

gabrielluong commented Apr 3, 2024

Also curious if there's a preference for the existing order of fillColor and pathData.

How I went about this was that I copied each individual xml into our project in Android Studio and ran format file. This will always format things so that the ordering of these properties follow alphabetical order so fillColor will always appear before pathData. To make it easier to import the XML from acorn-icons to our project going forward, I wanted to ensure every single individual XML is now formatted correctly.

Are the tools:ignore changes something we accidentally forgot or stripped away recently (we've implemented a new workflow so we're curious if it affected the format)? I know we're supposed to add tools:ignore="VectorPath" to complex paths but saw that your change had some different values assigned to this too (e.g. tools:ignore="VectorPath,VectorRaster"). Curious if there's something new we should be aware of.

I added some of the missing tools:ignore="VectorPath" that Android Studio complained about. We need VectorRaster for every <path> if tools:targetApi="n" appears in the XML. Technically, we didn't need to import this change back into acorn-icons. It is only necessary when we change the fillColor from #000 to use our color token @color/mozac_ui_icons_fill in our project (Example). However, bringing this change back into the original XML source will make this one less thing that we will have to do when importing new icons.

@JulianGaibler
Copy link
Member

Sorry for the noise, it seems our (relatively new) Github action not only broke in the meantime but also was never tested on pull request from forks 😅

I'm trying to fix this; worst case, I run it locally and push the changes.

@gabrielluong are there any problems with us using slightl different formatting (we just run prettier on the XML) than what Android Studio returns?

@JulianGaibler
Copy link
Member

Okay, it seems making commits on PRs that are based on forks is...complicated (if not impossible?). So I just ran the action locally and pushed. Sorry again for the noise!

@jsimplicio jsimplicio merged commit a47bc8e into FirefoxUX:main Apr 5, 2024
1 check passed
@jsimplicio jsimplicio changed the title Reformat android xml files and include the necessary tools:ignore v1.12.0 Apr 5, 2024
@jsimplicio
Copy link
Member

@gabrielluong Thanks! I retitled the PR and added a description to follow our format. Hope that's ok. Also merged and cut a new release.

I also added your explanation about VectorRaster to the wiki here https://github.com/FirefoxUX/acorn-icons/wiki/File-format-conventions.

Julian has left another question in here for you but overall this makes sense, thank you again.

@gabrielluong
Copy link
Collaborator Author

gabrielluong commented Apr 5, 2024

@gabrielluong are there any problems with us using slightl different formatting (we just run prettier on the XML) than what Android Studio returns?

I took a look at what was landed with the prettier formatting. I would strongly recommend going with the formatting that Android Studio returns. If we were diffing changes between what we imported into Android Studio (or in this case our fenix project) and what is in this repo, then I would prefer to not see formatting as being the difference because this will affects all files, which only makes this harder on us in terms of syncing changes. This is to avoid having folks importing XML from this repo into fenix and having to reformat the file manually. The worst case scenario is that we end up with 2 different formatting of these XMLs in fenix (either people run the reformatting or just commit as-is), which only results in a harder task of diffing the changes with this repo and keeping up to date with any icon changes. As long as Android Studio will keep formatting the XML file when we import the XML from this repo, then I have to abide with the Android Studio formatting.

My specific ask here will be to drop all the formatting changes that were added in the subsequent commits in which <vector xmlns:android="http://schemas.android.com/apk/res/android" are no longer on the same line. See e1cd3d6#diff-4a31bf25d58382dc5a12b7dbe8e85ed5503c1aaaea7c6298496b0c1bf15f75a3 and 7085ea2

Sorry for any inconvenience

jsimplicio pushed a commit that referenced this pull request Jun 24, 2024
* Reformat android xml files and include the necessary tools:ignore
* Updated github pull action from v3 to v4
* Trying to fix github action for pull requests from forks
* Ran Github Action manually

---------

Co-authored-by: JulianGaibler <[email protected]>
jsimplicio pushed a commit that referenced this pull request Jun 24, 2024
* Reformat android xml files and include the necessary tools:ignore
* Updated github pull action from v3 to v4
* Trying to fix github action for pull requests from forks
* Ran Github Action manually

---------

Co-authored-by: JulianGaibler <[email protected]>
@gabrielluong gabrielluong deleted the format branch July 19, 2024 04:18
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