[iOS][Identity snackbar]Move to SF enterprise icon [chromium/src : main]
Arthur Milchior (Gerrit)
Arthur Milchior added 1 comment![]( "Open in Gerrit")
Ewann, can you please review the symbol? Nicolas the use of the symbol in the snackbar.
Related details
- Ewann Pellé
- Nicolas Ouellet-Payeur
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas Ouellet-Payeur (Gerrit)
Nicolas Ouellet-Payeur added 5 comments![]( "Open in Gerrit")
https://drive.google.com/file/d/1kAg-tLG00DYGLYWxpFM3r59tb1csgq4m/view?usp=drive_linkThe icon in the screenshot is wrong: the left section has 5 "windows" instead of 8. The export from the SF Symbols app didn't work....
I sent you another SVG file in chat. It should fix the issue, can you try it out?
{
"images" : [
{
"idiom" : "universal",
"scale" : "1x"
},
{
"filename" : "legacy_cl...@2x.png",
"idiom" : "universal",
"scale" : "2x"
},
{
"filename" : "legacy_cl...@3x.png",
"idiom" : "universal",
"scale" : "3x"
}
],
"info" : {
"author" : "xcode",
"version" : 1
}
}
dirty worktree? Same for `legacy_cloud_and_arrow_up.imageset`
"Assets.xcassets/enterprise.symbolset/Contents.json",
"Assets.xcassets/enterprise.symbolset/enterprise.cr.svg",These files are missing from the CL
return SymbolWithPalette(CustomSymbolWithPointSize(kEnterpriseSymbol, 24.),nit: in the spirit of IWYU, add `#import "ios/chrome/browser/shared/ui/symbols/symbol_helpers.h"`
return SymbolWithPalette(CustomSymbolWithPointSize(kEnterpriseSymbol, 24.),Can you update the icon in `CentralAccountView` as well?
Related details
- Arthur Milchior
- Ewann Pellé
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Milchior (Gerrit)
Arthur Milchior voted and added 5 comments![]( "Open in Gerrit")
Votes added by Arthur Milchior
| Commit-Queue | +1 |
5 comments
https://drive.google.com/file/d/1kAg-tLG00DYGLYWxpFM3r59tb1csgq4m/view?usp=drive_linkThe icon in the screenshot is wrong: the left section has 5 "windows" instead of 8. The export from the SF Symbols app didn't work....
I sent you another SVG file in chat. It should fix the issue, can you try it out?
Done
"images" : [
{
"idiom" : "universal",
"scale" : "1x"
},
{
"filename" : "legacy_cl...@2x.png",
"idiom" : "universal",
"scale" : "2x"
},
{
"filename" : "legacy_cl...@3x.png",
"idiom" : "universal",
"scale" : "3x"
}
],
"info" : {
"author" : "xcode",
"version" : 1
}
}
dirty worktree? Same for `legacy_cloud_and_arrow_up.imageset`
No idea what occured. I guess I opened this one by accident in xcode and this had side effect. Not sure
"Assets.xcassets/enterprise.symbolset/Contents.json",
"Assets.xcassets/enterprise.symbolset/enterprise.cr.svg",These files are missing from the CL
Done
return SymbolWithPalette(CustomSymbolWithPointSize(kEnterpriseSymbol, 24.),nit: in the spirit of IWYU, add `#import "ios/chrome/browser/shared/ui/symbols/symbol_helpers.h"`
You’re not allowed to do that, it’s a private import. If you try, you’ll get an error message. If you look at it Builds file, you’ll see that only symbols.h is public.
return SymbolWithPalette(CustomSymbolWithPointSize(kEnterpriseSymbol, 24.),Can you update the icon in `CentralAccountView` as well?
I thought about doing it, but I don’t know which size exactly it should be or whether there are unexpected side effect. Honestly, I prefer it to be done in a separate CL. So that it’s possible to revert only one of the two if anything goes wrong
Related details
- Ewann Pellé
- Nicolas Ouellet-Payeur
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas Ouellet-Payeur (Gerrit)
Nicolas Ouellet-Payeur voted and added 5 comments![]( "Open in Gerrit")
Votes added by Nicolas Ouellet-Payeur
| Code-Review | +1 |
5 comments
LGTM, except for the dirty worktree
{This directory is a duplicate. You don't need `enterprise.cr.symbolset`
{
"images" : [
{
"idiom" : "universal",
"scale" : "1x"
},
{
"filename" : "legacy_cl...@2x.png",
"idiom" : "universal",
"scale" : "2x"
},
{
"filename" : "legacy_cl...@3x.png",
"idiom" : "universal",
"scale" : "3x"
}
],
"info" : {
"author" : "xcode",
"version" : 1
}
}
Arthur Milchiordirty worktree? Same for `legacy_cloud_and_arrow_up.imageset`
No idea what occured. I guess I opened this one by accident in xcode and this had side effect. Not sure
Still an issue in patchset 3
return SymbolWithPalette(CustomSymbolWithPointSize(kEnterpriseSymbol, 24.),Arthur Milchiornit: in the spirit of IWYU, add `#import "ios/chrome/browser/shared/ui/symbols/symbol_helpers.h"`
You’re not allowed to do that, it’s a private import. If you try, you’ll get an error message. If you look at it Builds file, you’ll see that only symbols.h is public.
Acknowledged
return SymbolWithPalette(CustomSymbolWithPointSize(kEnterpriseSymbol, 24.),Arthur MilchiorCan you update the icon in `CentralAccountView` as well?
I thought about doing it, but I don’t know which size exactly it should be or whether there are unexpected side effect. Honestly, I prefer it to be done in a separate CL. So that it’s possible to revert only one of the two if anything goes wrong
So that it’s possible to revert only one of the two if anything goes wrong
I don't really see the point in that, since we can always forward-fix. Your decision
Related details
- Arthur Milchior
- Ewann Pellé
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Milchior (Gerrit)
Arthur Milchior added 2 comments![]( "Open in Gerrit")
This directory is a duplicate. You don't need `enterprise.cr.symbolset`
Done
{
"images" : [
{
"idiom" : "universal",
"scale" : "1x"
},
{
"filename" : "legacy_cl...@2x.png",
"idiom" : "universal",
"scale" : "2x"
},
{
"filename" : "legacy_cl...@3x.png",
"idiom" : "universal",
"scale" : "3x"
}
],
"info" : {
"author" : "xcode",
"version" : 1
}
}
Arthur Milchiordirty worktree? Same for `legacy_cloud_and_arrow_up.imageset`
Nicolas Ouellet-PayeurNo idea what occured. I guess I opened this one by accident in xcode and this had side effect. Not sure
Still an issue in patchset 3
- Ewann Pellé
- Nicolas Ouellet-Payeur
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas Ouellet-Payeur (Gerrit)
Nicolas Ouellet-Payeur added 1 comment![]( "Open in Gerrit")
Related details
- Arthur Milchior
- Ewann Pellé
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
- Arthur Milchior
- Ewann Pellé
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ewann Pellé (Gerrit)
Ewann Pellé added 3 comments![]( "Open in Gerrit")
[iOS][Identity snackbar]Move to SF enterprise iconoptional nit: symbols
https://drive.google.com/file/d/1n7sNgpfWosdntBJeffM7h40xkYyS4hF-/view?usp=drive_linkDo you know if this symbol is copyrighted, if yes, you should add it in ios_internal.
return SymbolWithPalette(CustomSymbolWithPointSize(kEnterpriseSymbol, 24.),Please use constants, not literals.
Related details
- Arthur Milchior
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |