GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — feat(): local icon handling in actionbar and tabview by MartoYankov · Pull Request #7009 · NativeScript/NativeScript

Conversation

@MartoYankov

@MartoYankov MartoYankov commented Mar 8, 2019

Copy link
Copy Markdown
Contributor

PR Checklist

  • The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
  • There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
  • You have signed the CLA.
  • All existing tests are passing: https://github.com/NativeScript/NativeScript/blob/master/DevelopmentWorkflow.md#running-unit-tests.
  • Tests for the changes are included - https://github.com/NativeScript/NativeScript/blob/master/WritingUnitTests.md.

What is the current behavior?

On Android, icons rendered in TabView and Actionbar are not scaled correctly due to the use of a deprecated native Android constructor https://developer.android.com/reference/android/graphics/drawable/BitmapDrawable#BitmapDrawable(android.graphics.Bitmap).

On iOS, there was no property on ActionBar to specify icon rendering mode and it is hard coded to alwaysOriginal, which made the use of image icons limited.

What is the new behavior?

Fixed the Android issue with the use of the new constructor.

Added iosIconRenderingMode on ActionBar. To avoid breaking changes, on ActionBar this property defaults to alwaysOriginal and on TabView, it defaults to automatic. We should probably change this for 6.0.

Fixes #5887

Tests

  • Added 2 new e2e tests for Action Bar - actLocalIcons and actResIcons.
  • Added 1 new e2e test for TabView - tab-view-icons-local

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@MartoYankov MartoYankov added e2e test needed docs needed Additional documentation on this issue/PR is needed labels Mar 8, 2019
@MartoYankov MartoYankov self-assigned this Mar 8, 2019
@MartoYankov
MartoYankov requested review from ADjenkov, manoldonev and vakrilov March 8, 2019 13:31
@cla-bot cla-bot Bot added the cla: yes label Mar 8, 2019
@ghost ghost added the in progress label Mar 8, 2019
@MartoYankov
feat(): local icon handling in actionbar and tabview
4206704
@MartoYankov
MartoYankov force-pushed the myankov/android-icons branch from 0d711d3 to 4206704 Compare March 8, 2019 14:09
@ghost ghost assigned SvetoslavTsenov Mar 8, 2019
@SvetoslavTsenov @MartoYankov
Merge branch 'master' into myankov/android-icons
89c96c8
@MartoYankov
MartoYankov force-pushed the myankov/android-icons branch from c69fe70 to 89c96c8 Compare March 11, 2019 11:48
@SvetoslavTsenov
Merge branch 'master' into myankov/android-icons
0a85a42
@SvetoslavTsenov

SvetoslavTsenov commented Mar 12, 2019

Copy link
Copy Markdown
Contributor

test ios#/tns-dist/tns-ios/PR/e34f49eb5af64946cf1257bd2b39760518235427/tns-ios.tgz

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

manoldonev
manoldonev reviewed Mar 12, 2019
View reviewed changes
Comment thread tns-core-modules/ui/action-bar/action-bar.ios.ts
} else if (item.icon) {
const img = loadActionIconFromFileOrResource(item.icon);
barButtonItem = UIBarButtonItem.alloc().initWithImageStyleTargetAction(img, UIBarButtonItemStyle.Plain, tapHandler, "tap");
const image = img.imageWithRenderingMode(this._getIconRenderingMode());

@manoldonev manoldonev Mar 12, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Guard the img usage as here

NativeScript/tns-core-modules/ui/action-bar/action-bar.ios.ts

Line 188 in 0a85a42

if (img) {
?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
manoldonev
manoldonev approved these changes Mar 12, 2019
View reviewed changes
@SvetoslavTsenov
SvetoslavTsenov merged commit cd66300 into master Mar 12, 2019
@ghost ghost removed the in progress label Mar 12, 2019
@SvetoslavTsenov
SvetoslavTsenov deleted the myankov/android-icons branch March 12, 2019 13:08
@SvetoslavTsenov SvetoslavTsenov removed the e2e test needed label Mar 13, 2019
@lock

lock Bot commented Mar 17, 2020

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@lock lock Bot locked and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@vakrilov vakrilov Awaiting requested review from vakrilov
@ADjenkov ADjenkov Awaiting requested review from ADjenkov
1 more reviewer
@manoldonev manoldonev manoldonev approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@SvetoslavTsenov SvetoslavTsenov

@MartoYankov MartoYankov

Labels

cla: yes docs needed Additional documentation on this issue/PR is needed

Projects

NativeScript Docs Project
Status: 🆕 New

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Android uses deprecated method to create Drawable from Bitmap

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@MartoYankov @SvetoslavTsenov @manoldonev
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.