-
Notifications
You must be signed in to change notification settings - Fork 88
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.
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
Get rid of the terms skin
, modern
and default
#670
Get rid of the terms skin
, modern
and default
#670
Conversation
* slightly restructure the file structure to better structure it
@@ -1 +1,107 @@ | |||
@import 'skin-modern/skin'; |
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.
Since we will only have one UI look and feel going forward (like now) there is no need anymore to add this extra level of 'abstraction'. I simply copied the content of _skin.scss
file directly into this one. The code was not changed.
selectedOptionLabelText = selectedOptionLabelText + ' (' + (availableSettings - 1) + ')'; | ||
selectedOptionLabelText = i18n.performLocalization(selectedOptionLabelText) + ' (' + (availableSettings - 1) + ')'; |
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.
This was a linter error as the + operator is not allowed on the LocalizableText
type. This is actually a bug fix.
src/ts/demofactory.ts
Outdated
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.
The DemoFactory was fully removed as it only adds complexity and we have the very same code already in our player-web samples. This was never meant to be used publicly.
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.
This file got slightly restructured but no actual code changes expect method names renaming - removing the default and modern terms.
The structure now is the following:
- UIFactory (Contains all methods which should be used publicly)
- buildUI
- buildSmallScreenUI
- buildCastReceiverUI
- buildTvUI
- private top level functions which builds only the UIContainer.
This change was done as it was always confusing what is the difference between buildSmallScreenUI
and smallScreenUI
functions.
In an effort to futhre reduce complexly I renamed the functions which only build the UIContainer
and added the Layout
suffix.
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.
LGTM, just have some NITs
Description
For our new UI we want to cleanup the usage of the terms
skin
,default
andmodern
since none of them was really in use.default
or themodern
one (which are the same) therefore there is no point in keeping this naming scheme. Also if we ever introduce another skin it's hard to come up with a matching name aside modern.Don't be afraid of the PR size. Most of it is just renaming and moving Code around. All other changes are explicitly highlighted below.
Those changes are API breaking so pleas review with that in mind‼️