Skip to content

Conversation

manoldonev
Copy link
Contributor

No description provided.

@manoldonev manoldonev self-assigned this Aug 22, 2019
@cla-bot cla-bot bot added the cla: yes label Aug 22, 2019
@manoldonev manoldonev force-pushed the mdonev/migrate-to-bottomnav branch 2 times, most recently from 96a42a9 to da54097 Compare August 22, 2019 16:46
@manoldonev manoldonev force-pushed the mdonev/migrate-to-bottomnav branch from da54097 to 86c3ae6 Compare August 23, 2019 10:42
@manoldonev manoldonev force-pushed the mdonev/migrate-to-bottomnav branch from bb88098 to 5be380b Compare August 23, 2019 12:23
@manoldonev manoldonev changed the title [WIP] feat(tab): migrate to bottomnavigation component [WIP] feat(tab): migrate template to use BottomNavigation component Aug 23, 2019
@manoldonev manoldonev changed the title [WIP] feat(tab): migrate template to use BottomNavigation component feat(tab): migrate template to use BottomNavigation component Aug 23, 2019
*tabItem="{title: 'Home', iconSource: getIconSource('home')}"
name="homeTab">
</page-router-outlet>
<TabStripItem title="Home" iconSource="font://&#xf015;" />
Copy link
Contributor

@MartoYankov MartoYankov Aug 27, 2019

Choose a reason for hiding this comment

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

I don't think we should promote this syntax when you use font icons. It's highly unusual to want the title to be with a font icon font. This syntax is useful when you want to set an image from the resources as an icon source and even then, it's simply a shorthand syntax. The longer more detailed syntax still works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, actually thought the same yesterday while explaining pros&cons of each syntax to the team -- forgot to update the comment with app resource value :)

}
}

TabStripItem Label {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can do this whole section with the TabStripItem only since the color property is inherited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually left it for consistency as platform-specific css needs to set text-transform and (as discussed previously) this is not inherited css property due to historic reasons so we need to use TabStripItem Label { ... } selector there.

@manoldonev manoldonev force-pushed the mdonev/migrate-to-bottomnav branch from c2d20eb to e6d53ea Compare August 27, 2019 14:45
@manoldonev manoldonev marked this pull request as ready for review August 28, 2019 12:33
@manoldonev manoldonev merged commit 4483059 into master Aug 28, 2019
@manoldonev manoldonev deleted the mdonev/migrate-to-bottomnav branch August 28, 2019 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants