Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

For #6102 : Add lint checks for using app namespace for vector drawables #6112

Merged
merged 2 commits into from Mar 2, 2020
Merged

For #6102 : Add lint checks for using app namespace for vector drawables #6112

merged 2 commits into from Mar 2, 2020

Conversation

ekager
Copy link
Contributor

@ekager ekager commented Feb 28, 2020 •
edited

So every few weeks it seems like I end up having to update new features in either Fenix or AC to use the app namespace instead of the android namespace for drawables so we don't get wonky behavior on older devices. I was thinking a custom lint rule could help me out!
Assuming we like the idea I have a few questions!

  1. Can/will Fenix and other consumers pick up these lint checks?
  2. Should we lower the severity? I want people to actually see it so I'm not sure a warning is as effective but I also don't want to break other consumers' layout files.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@ekager ekager added the 🕵️‍♀️ needs review PRs that need to be reviewed label Feb 28, 2020
@codecov
Copy link

codecov bot commented Feb 28, 2020 •
edited

Codecov Report

Merging #6112 into master will decrease coverage by 0.23%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6112      +/-   ##
============================================
- Coverage     78.33%   78.09%   -0.24%     
- Complexity     4477     4480       +3     
============================================
  Files           585      593       +8     
  Lines         21492    21662     +170     
  Branches       3125     3156      +31     
============================================
+ Hits          16836    16918      +82     
- Misses         3378     3444      +66     
- Partials       1278     1300      +22
Impacted Files Coverage Δ Complexity Δ
...la/components/feature/session/FullScreenFeature.kt
...ponents/feature/session/PictureInPictureFeature.kt
.../components/feature/session/EngineViewPresenter.kt
...ents/feature/session/CoordinateScrollingFeature.kt
...ents/feature/session/TrackingProtectionUseCases.kt
...zilla/components/feature/session/SessionFeature.kt
...illa/components/feature/session/HistoryDelegate.kt
...la/components/feature/session/ThumbnailsFeature.kt
...illa/components/feature/session/SessionUseCases.kt
...ature/session/behavior/EngineViewBottomBehavior.kt
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9e89d0...2712336. Read the comment docs.

@ekager ekager force-pushed the src-compat branch 3 times, most recently from a98986c to b270b4d Compare March 2, 2020 04:31
@ekager ekager mentioned this pull request Mar 2, 2020
Merged
6 tasks
pocmo
pocmo approved these changes Mar 2, 2020
View reviewed changes
Copy link
Contributor

@pocmo pocmo left a comment

Choose a reason for hiding this comment

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

💯

That's a great lint check!

bors r+

@pocmo pocmo added 🛬 needs landing PRs that are ready to land and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Mar 2, 2020
@pocmo pocmo self-assigned this Mar 2, 2020
bors bot pushed a commit that referenced this pull request Mar 2, 2020
@MozLando @ekager
Merge #6112 #6136
779be4c 
6112: For #6102 : Add lint checks for using app namespace for vector drawables r=pocmo a=ekager




6136: For #6102 - When tinting ImageViews, use AppCompatImageView and app:tint r=pocmo a=ekager



Co-authored-by: ekager <ekager@mozilla.com>
@bors
Copy link

bors bot commented Mar 2, 2020

Build failed (retrying...)

  • complete-push

bors bot pushed a commit that referenced this pull request Mar 2, 2020
@MozLando @ekager
Merge #6112
aa1bb75 
6112: For #6102 : Add lint checks for using app namespace for vector drawables r=pocmo a=ekager




Co-authored-by: ekager <ekager@mozilla.com>
@bors
Copy link

bors bot commented Mar 2, 2020

Build failed

@ekager
Copy link
Contributor Author

ekager commented Mar 2, 2020

bors retry

@bors
Copy link

bors bot commented Mar 2, 2020

Build succeeded

  • complete-push

@bors bors bot merged commit 511b20b into mozilla-mobile:master Mar 2, 2020
@ekager ekager deleted the src-compat branch March 2, 2020 17:56
@ekager ekager mentioned this pull request Mar 3, 2020
Closed
@ekager ekager mentioned this pull request Mar 21, 2020
Merged
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pocmo pocmo pocmo approved these changes

Assignees

@pocmo pocmo

Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ekager @pocmo