Skip to content
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

WRP-26847: Create theming "Hello" App #206

Draft
wants to merge 25 commits into
base: develop
Choose a base branch
from
Draft

WRP-26847: Create theming "Hello" App #206

wants to merge 25 commits into from

Conversation

alexandrumorariu
Copy link
Contributor

@alexandrumorariu alexandrumorariu commented Sep 15, 2023 •
edited by adrian-cocoara-lgp
Loading

Enact-DCO-1.0-Signed-off-by: Alexandru Morariu [email protected]

Checklist

  • I have read and understand the contribution guide
  • A CHANGELOG entry is included
  • At least one test case is included for this feature or bug fix
  • Documentation was added or is not needed
  • This is an API breaking change

Issue Resolved / Feature Added

Create an app "themeing hello app" for changing color and do the documentation based on that app. See custom skin sample.
We need 2 separate panels (or view).
On the first view we should have all the presets
On the second view there will be the css variables customization.
Beside these view, there should be a live preview section, on the right side of the screen, just like in custom skin samples

Resolution

Additional Considerations

Renamed custom-skin sample to feature-custom-skin-generator after internal discussion

Links

WRP-26847

Comments

@alexandrumorariu alexandrumorariu changed the title WRP-26847 WRP-26847: Create theming "Hello" App Sep 15, 2023
adrian-cocoara-lgp
adrian-cocoara-lgp approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@adrian-cocoara-lgp adrian-cocoara-lgp left a comment

Choose a reason for hiding this comment

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

👍

daniel-stoian-lgp
daniel-stoian-lgp approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@daniel-stoian-lgp daniel-stoian-lgp left a comment

Choose a reason for hiding this comment

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

LGTM

alexandrumorariu and others added 4 commits September 21, 2023 10:15
@alexandrumorariu
Change name of files
cb9bb51
@alexandrumorariu
Merge branch 'feature/WRP-26847' of https://github.com/enactjs/samples …
3129f9a 
…into feature/WRP-26847
@stanca-pop-lgp
WRP-28251: Create documentation for dynamic color mode (#208)
bea167b 
* added jsdocs, comments and readme

* change sample name in readme

* fixed explanation in readme

* minor fix in readme
@adrian-cocoara-lgp
WRP-26847: Save colors/presets using LS2 Request (#209)
35a7a44 
* added ls2Request to save and load presets/colors

* renamed sample folder to `pattern-ls2request-custom-colors`

* fixed initial render bug to not display default colors while data is loading

* deleted unnecessary `console.log`

* added comments about luna service calls

* fixed lint warnings
0x64
0x64 reviewed Oct 17, 2023
View reviewed changes
sandstone/pattern-ls2request-custom-colors/screenTypes.json Show resolved Hide resolved
sandstone/pattern-ls2request-custom-colors/src/App/App.js Outdated Show resolved Hide resolved
sandstone/pattern-ls2request-custom-colors/src/views/CustomizePanel/CustomizePanel.js Outdated Show resolved Hide resolved
sandstone/pattern-ls2request-custom-colors/src/views/PresetPanel/PresetPanel.js Outdated Show resolved Hide resolved
sandstone/pattern-ls2request-custom-colors/src/views/PresetPanel/PresetPanel.js Outdated Show resolved Hide resolved
sandstone/pattern-ls2request-custom-colors/src/hooks/dynamicColor.js Outdated Show resolved Hide resolved
sandstone/pattern-ls2request-custom-colors/src/constants.js Outdated Show resolved Hide resolved
sandstone/pattern-ls2request-custom-colors/src/views/MainView/MainView.js Outdated

useEffect(() => {
if (isWebOSPlatform) {
getSettings({
Copy link
Member

Choose a reason for hiding this comment

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

Do we need subscribe option here? Does the current code get updated settings value during this app is active?

Copy link
Contributor

Choose a reason for hiding this comment

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

This getSettings call was added to fix a render issue. At first render, or when the app was refreshed, the default sandstone colors were applied to the app until the values from theme key were fetched. So I used this call to conditionally render a loading spinner until the theme key data is done fetching. I'll see if I can find another way to fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to remove the subscription from here.

sandstone/pattern-ls2request-custom-colors/src/views/MainView/MainView.js Outdated
const [panelIndex, setPanelIndex] = useState(0);

// when running on webOS system, wait for `theme` key data to be available, then render the view
const isWebOSPlatform = platform.tv;
Copy link
Member

Choose a reason for hiding this comment

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

It is not safe to check platform.tv as there so many versions of webOS. Try-and-error style approach will be better. Or, we may be possible to check the availability via LS2Request first.
If we change like above, MainView has to render Panels when settings service is not available.

Copy link
Contributor

Choose a reason for hiding this comment

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

This part is removed now

sandstone/pattern-ls2request-custom-colors/src/views/PresetPanel/PresetPanel.js Outdated
changeSettings({
category: 'customUi',
settings: {
theme: JSON.stringify(newContext)
Copy link
Member

Choose a reason for hiding this comment

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

It looks better to describe how 'newContext' looks like for app developers. (In comments or any other manner...)

Copy link
Contributor

Choose a reason for hiding this comment

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

I added some explanatory comments

@0x64
Copy link
Member

0x64 commented Oct 17, 2023

Our goal is to guide app developers to know below.

  • How to colorize an app
    • How to get values from settings service in launching and run-time
      (including to ignore settings service on unsupported platforms)
    • How to generate CSS variables based on the fetched values
    • How to apply generated CSS variables to an app
  • How to save custom skin
    • What values are possible
    • How to set values to settings service

Code in this app has a role of recommended snippet. So, parts for guide need to be separated for readers.

seunghoh
seunghoh requested changes Oct 31, 2023
View reviewed changes
Copy link
Member

@seunghoh seunghoh left a comment

Choose a reason for hiding this comment

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

I reviewed it assuming I was an external developer who knew nothing about the content, but it was so complicated that I didn't know where to start following the code.
I am afraid to say that the PR is quite far from my expectations :(
This is not the sample application, should be a code pattern or code snippet. I don't want to move the whole part of all features from the settings app. Let's focus on custom skin save/load via using setting service. These are some comments that I'd like to modify.

  • I'd like to have key files explanation in the md.
  • Remove Active dynamic color mode (keep this feature separately for the future but not in this sample)
  • Remove Adjust skin automatically (keep this feature separately for the future but not in this sample)
  • Remove Theme Preview from the preset and customize panel. Apply skins into only Theme Preview and overriding other area is too complicated. Let's update the Whole area.
  • Add new Load theme panel separately so that the app can simply refer to the third view for those who need only load the skin.

Looks like a whole refactoring or redevelopment. If you feel uncomfortable to do this. It's totally OK to say no. Then, I will reassign it to my team.

sandstone/pattern-ls2request-custom-colors/README.md
@@ -0,0 +1,20 @@
## Custom Colors Generator
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Custom Colors Generator
## Custom Colors LS2Request pattern

sandstone/pattern-ls2request-custom-colors/README.md
@@ -0,0 +1,20 @@
## Custom Colors Generator

A sample Enact application that uses dynamic color change feature to style components and create a personalized theme.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A sample Enact application that uses dynamic color change feature to style components and create a personalized theme.
A sample Enact application that how to customize a Sandstone color, save & load colors via webos setting service.

sandstone/pattern-ls2request-custom-colors/README.md

#### Reset Theme

The `Reset` button will revert all the changes to the active selected preset.
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to have a simple file structure and explanation of important file

sandstone/pattern-ls2request-custom-colors/src/views/MainView/MainView.js
skin={applySkin ? skin : 'neutral'}
>
<Panel>
<Header className={css.panelHeader} subtitle="Choose preset theme" title="Theming App">
Copy link
Member

Choose a reason for hiding this comment

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

Theming App -> Theme

sandstone/pattern-ls2request-custom-colors/src/views/MainView/MainView.js
<PresetPanel />
</Panel>
<Panel>
<Header className={css.panelHeader} subtitle="Customize theme" title="Theming App" />
Copy link
Member

Choose a reason for hiding this comment

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

Theming App -> Theme

sandstone/pattern-ls2request-custom-colors/src/views/MainView/MainView.js
<Panel>
<Header className={css.panelHeader} subtitle="Customize theme" title="Theming App" />
<CustomizePanel />
</Panel>
Copy link
Member

Choose a reason for hiding this comment

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

Add one more panel title "Theme" and subtitle "Load theme".
Theme content can be what you already made in the preview section

@daniel-stoian-lgp daniel-stoian-lgp marked this pull request as draft November 6, 2023 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0x64 0x64 0x64 left review comments

@adrian-cocoara-lgp adrian-cocoara-lgp adrian-cocoara-lgp approved these changes

@seunghoh seunghoh seunghoh requested changes

@daniel-stoian-lgp daniel-stoian-lgp daniel-stoian-lgp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alexandrumorariu @0x64 @seunghoh @daniel-stoian-lgp @adrian-cocoara-lgp @stanca-pop-lgp