8000 fix(android/action-bar): process Icon Fonts in NavigationButton the same way as in ActionItem by ycherniavskyi · Pull Request #7842 · NativeScript/NativeScript · GitHub
[go: up one dir, main page]
Skip to content

fix(android/action-bar): process Icon Fonts in NavigationButton the same way as in ActionItem #7842

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 25, 2019
Merged

fix(android/action-bar): process Icon Fonts in NavigationButton the same way as in ActionItem #7842

merged 2 commits into from
Sep 25, 2019

Conversation

ycherniavskyi
Copy link
Contributor

PR Checklist

What is the current behavior?

NavigationButton does not process font:// prefix in icon field.

What is the new behavior?

NavigationButton processes font:// prefix in icon field, the same way as ActionItem does.

Unfortunately, I could not found any test fro this functionality for ActionItem, that is why I could not implement the correct one for NavigationButton.

@cla-bot cla-bot bot added the cla: yes label Sep 18, 2019
@MartoYankov MartoYankov self-assigned this Sep 20, 2019
@MartoYankov
Copy link
Contributor

@ycherniavskyi Thanks for the PR! There are UI tests with appium for this functionality. They are located in the /e2e/ui-tests-app app. The example is located here and the actual test here. Let me know if you have any questions.

@ycherniavskyi ycherniavskyi force-pushed the process-icon-fonts-in-NavigationButton branch from 49d9105 to 48555fa Compare September 20, 2019 07:30
@ycherniavskyi
Copy link
Contributor Author

@MartoYankov thank you for pointing relative test location. As I understand current action-bar: font-icons test just render the page and it must be rendered without error. So I just add NavigationButton with an icon which uses icon fonts. If my test is incorrect or incomplete, could you please explain in more detail how exactly it must be implemented.

I also rebase PR on the latest master branch.

@MartoYankov
Copy link
Contributor

Yes, that's basically it.

@MartoYankov
Copy link
Contributor

test

@ycherniavskyi
Copy link
Contributor Author

@MartoYankov could you please give me CI error of my PR? For some reason, I get the error (nsbuild01.telerik.com’s server IP address could not be found) when trying to reach http://nsbuild01.telerik.com:8080/build/job/pr-composite-ns-core-modules/2439.

@MartoYankov
Copy link
Contributor

@ycherniavskyi We had some problems with the CI and it was stopped. I'll re-run it.

@MartoYankov
Copy link
Contributor

test

@ycherniavskyi
Copy link
Contributor Author
ycherniavskyi commented Sep 24, 2019 •
edited
Loading

I differently messed up something with the test. Because from CI error comparing image, I can see correctly rendered Icon Fonts for NavigationButton on the actual image.

action-bar-font-icons_diff

Unfortunately, it isn't clear for me what exactly driver.imageHelper.compareScreen() do. What screen compare to what screen. And if the actual image is correct how and where the comparing image generates.

@MartoYankov could you please give me some tips how I neet fo update test, so it passed?

@MartoYankov
Copy link
Contributor

@ycherniavskyi It's expected to have such failing images with our current setup. The compare is to the last passed image from master branch. The idea is that we have to check it and approve the new image if it looks fine.

In this case it looks odd that the icons in the ActionItems and the icon in the NavigationButton have the same font size, but the NavigationButton looks so much bigger. I'll have a look into this.

@ycherniavskyi
Copy link
Contributor Author

@MartoYankov thank you for clarification. I basically understand the e2e test technic now.

As for NavigationButton icon size. It seems that the real issue is some size constraints or scaling of ActionItem element. Because after reach some font-size, the size of ActionItem icon stop changing when the size of NavigationButton icon continues growing.

@MartoYankov
Copy link
Contributor

@ycherniavskyi It looks like you are right. It's also appears to be done on the native side of the Toolbar widget.

@MartoYankov MartoYankov added the docs needed Additional documentation on this issue/PR is needed label Sep 25, 2019
@MartoYankov MartoYankov merged commit 4991e6d into NativeScript:master Sep 25, 2019
@GulajavaMinistudio GulajavaMinistudio mentioned this pull request Sep 26, 2019
Merged
5 tasks
@manoldonev manoldonev mentioned this pull request Oct 8, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartoYankov MartoYankov MartoYankov approved these changes

Assignees

@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.
2 participants
@ycherniavskyi @MartoYankov
0