feat: enable to set theme for the scene #67

issmail-basel · 2021-08-23T06:51:08Z

Overview

Add Scene theme option, users can define the basic theme which is used for all components on the scene.

What I've done

Add pre made theme colors
Add option to change used theme/colors in scene tab
Calculate colors for custom theme

How I tested

Change options in scene tab -> Publish Theme
Choose pre made theme or custom theme with color preferences
Check changes in the Editor, Preview page and published project

Screenshot

Which point I want you to review particularly

Ways to improve code quality
Any missing styles

netlify · 2021-08-23T06:51:14Z

✔️ Deploy Preview for reearth-web ready!

🔨 Explore the source changes: 4ff8efd

🔍 Inspect the deploy log: https://app.netlify.com/sites/reearth-web/deploys/613af9a8a19b930008e5fe5e

😎 Browse the preview: https://deploy-preview-67--reearth-web.netlify.app

codecov · 2021-08-23T06:54:44Z

Codecov Report

Merging #67 (4ff8efd) into main (481df59) will decrease coverage by 0.63%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main      #67      +/-   ##
==========================================
- Coverage   58.10%   57.46%   -0.64%     
==========================================
  Files          40       44       +4     
  Lines         685      703      +18     
  Branches       96      104       +8     
==========================================
+ Hits          398      404       +6     
- Misses        245      257      +12     
  Partials       42       42

Impacted Files	Coverage Δ
src/theme/colors.ts	`100.00% <ø> (ø)`
src/theme/publishTheme/index.ts	`20.00% <20.00%> (ø)`
src/theme/publishTheme/dark.ts	`100.00% <100.00%> (ø)`
src/theme/publishTheme/forest.ts	`100.00% <100.00%> (ø)`
src/theme/publishTheme/light.ts	`100.00% <100.00%> (ø)`

issmail-basel · 2021-08-23T06:54:46Z

@lavalse , what about menu colors, should they stay dark?
if not please update figma to take care of missing parts

lavalse · 2021-08-24T03:01:33Z

@issmail-basel For the menu widgets, because we will refine the logic in the future. So for now,

this is the solution. They should also use the theme by default I think. Is that ok?

rot1024

This is a somewhat roundabout implementation:

Current: SceneProperty in Visualizer -up-> Global theme context -down-> Visualizer -down-> Each component (What was inside goes outside and then back inside again)
Should be: Visualizer (SceneProperty) -down-> Each component

Since public themes are used only inside Visualizer, they do not depend on the global theme context (@reearth/theme) and should be used by defining a premade theme inside Visualizer.

src/components/molecules/Visualizer/Engine/index.tsx

…e-front

src/components/molecules/Visualizer/Infobox/Frame/index.tsx

KaWaite · 2021-08-30T08:23:54Z

src/components/molecules/Visualizer/Widget/Menu/MenuButton.tsx

@@ -38,10 +38,12 @@ export type Props = {
  button: Button;
  menuItems?: MenuItem[];
  pos: Position;
+  sceneProperty?: any;


Suggested change

sceneProperty?: any;

sceneProperty?: SceneProperty;

Also needs the SceneProperty imported

This still isn't resolved.

src/components/molecules/Visualizer/Widget/Menu/MenuButton.tsx

src/components/molecules/Visualizer/Widget/Storytelling/index.tsx

KaWaite · 2021-08-30T08:41:53Z

src/theme/publishTheme/hooks.ts

+export default (seneThemeOptions?: ReTheme) => {
+  const [publishedTheme, setPublishedTheme] = useState<PublishTheme>(dark);
+  function addAlpha(color?: string, opacity?: number): string {
+    // coerce values so ti is between 0 and 1.


Suggested change

// coerce values so ti is between 0 and 1.

// coerce values so it is between 0 and 1.

Still doesn't seem to be updated.

Still not resolved

KaWaite · 2021-08-30T08:43:11Z

src/theme/publishTheme/hooks.ts

+  themeBackgroundColor?: string;
+};
+
+export default (seneThemeOptions?: ReTheme) => {


seneThemeOptions => sceneThemeOptions

Not resolved

src/theme/publishTheme/hooks.ts

KaWaite · 2021-08-31T01:26:29Z

src/components/molecules/Visualizer/Widget/Storytelling/index.tsx

@@ -174,22 +194,23 @@ const Title = styled(Text)`
  }
 `;

-const StyledIcon = styled(Icon)`
-  color: ${props => props.theme.main.text};
+const StyledIcon = styled(Icon)<{ publishedTheme: PublishTheme }>`


Sorry, one more thing. In components like this StyledIcon where all you are using is mainIcon from publishedTheme, can you pass JUST what you need, rather than the whole publishedTheme object.
In this case you could pass a color=publishedTheme.mainIcon prop instead.
Please update other places where this applies.

This wasn't addressed. Please let me know if you don't catch what I mean and I can explain more!

KaWaite · 2021-09-01T09:46:41Z

src/components/molecules/Visualizer/Widget/Storytelling/index.tsx

@@ -160,7 +179,8 @@ const Current = styled(Flex)`
  }
 `;

-const Title = styled(Text)`
+const Title = styled(Text)<{ color: string }>`
+  color: ${({ color }) => color}!important;


The !important isn't necessary. Above where you use Title just pass customColor and you should be good to go!
ie

<Title color={publishedTheme.mainText} size="m" weight="bold" customColor> ....

KaWaite

Just one change requested and then LGTM! I'll approve but please make the change!

rot1024

usePublicTheme seems redundant, and I don't see the need to use Hooks. It's enough to simply have the following code:

@reearth/theme/public.ts:

export type Theme = { themeBackgroundColor: string, /*...*/ };
export type ThemeType = "dark" | "light" | "forest";
export type ThemeConfig = Partial<Theme> & { type?: ThemeType | "custom" };

const defaultTheme: ThemeType = "dark";
const premade: {[key in ThemeType]: Theme } = {
  dark: { ... },
  light: { ... },
  forest: { ... }
};

export function publicTheme(theme?: ThemeConfig): Theme {
  return theme?.type === "custom"
    ? { ...premade[defaultTheme], ...theme }
    : premade[theme?.type || defaultTheme];
}

Usage:

import { publicTheme } from "@reearth/theme";

function Storytelling({ sceneProperty }) {
  const theme = publicTheme(sceneProperty?.theme);

  // use theme!
}

This code doesn't use hooks so the code is very clean.

issmail-basel · 2021-09-06T13:34:24Z

@rot1024 , thank you for your comment.
but hooks are needed to detect theme changes.
Your code is working when the component is rendered for the first time, but if we change the pre-made theme to (light for example) it won't detect the change.
That's why I've used hooks (useEffect).
What do you recommend to solve this issue? Should I keep using hooks?

rot1024 · 2021-09-07T09:03:33Z

It's strange. In my code, theme should be switched when sceneProperty is updated. Let me try my code in this PR.

rot1024 · 2021-09-08T03:48:44Z

OK, and I tried and I understand hooks is needed because custom theme uses object and it's needed to prevent useless re-rendering. But there are rooms to be refactored so I did.

Please check:

scene-theme-front...scene-theme-front-refactor

Points:

useState and useEffect can be replaced with useMemo
Separate the logic into hooks and a function that returns a theme object
Non-unified name: PublishTheme and PublishedTheme, I unified them with PublishTheme
Simpler file name and fewer files at src/theme

Co-authored-by: rot1024 <aayhrot@gmail.com>

* feat: tag system domain models (#39) * feat: tag system domain models * refactor: * add tag interface * tag -> group and tag->item conversation * testing: generate test cases for the tagID * resolve notes * fix unit tests errors * add NewId test code fix NewId func * add more test cases refactor some parts * feat: tag system data-layer (mongo) (#44) * feat: tag system data-layer (mongo) * remove len > 0 check * goimport * Update pkg/tag/group_builder.go Co-authored-by: rot1024 <aayhrot@gmail.com> * Update pkg/tag/item_builder.go Co-authored-by: rot1024 <aayhrot@gmail.com> * rename itemFrom and groupFrom funcs Co-authored-by: rot1024 <aayhrot@gmail.com> * feat: create tag group and tag item (#45) * tag item and group schema * feat: creat tags (GQL schema) * tag items and tag groups resolvers * datalayer (dummy memory) and usecases * receive list by reference * check if nil for list * resolve notes * generate new models * feat: memory infrastructure (#46) * refactor: implement memory infrastructure * test: implement memory infrastructure test cases * test: fix FindByScene test case * feat: attach/detach tag from layer (#50) * tag item and group schema * feat: creat tags (GQL schema) * tag items and tag groups resolvers * datalayer (dummy memory) and usecases * receive list by reference * check if nil for list * feat: introduce tags to layers * feat: attach/detach tags from layers * fix imports * refactor: resolve notes * test: test units for tags * refactor: resolve notes * feat: attach/detach tag item from group (#52) * refactor: transform group tags list to reference * feat: attach/detach tags * refactor: use params as use-case input * test: mongodoc testing (#61) * test: mongodoc testing * resolve notes * feat: remove tag (#58) * feat: remove a tag (init) * feat: remove tag * feat: remove tag usecase and infra * resolve notes * feat: tag system queries (#54) * feat: tag system queries * resolve notes * feat: update tag (#49) * tag item and group schema * feat: creat tags (GQL schema) * tag items and tag groups resolvers * datalayer (dummy memory) and usecases * receive list by reference * check if nil for list * feat: rename tag group * refactor: rename -> update * resolve notes * resolve notes * change find by id func return type * Merge branch 'tag-system' of https://github.com/reearth/reearth-backend into tag/update-group-label # Conflicts: # internal/adapter/gql/generated.go # internal/adapter/gql/gqlmodel/convert_tag.go * fix testing * resolve notes * resolve notes * resolve notes Co-authored-by: HideBa <baba.papa1120.ba@gmail.com> Co-authored-by: rot1024 <aayhrot@gmail.com>

Basel-Issmail added 2 commits August 21, 2021 23:22

Add scene theme

83c9505

Set main color of floating panel

1c08552

issmail-basel changed the title ~~Scene theme front~~ feat: enable to set theme for the scene Aug 23, 2021

Add default values

b6cfbd8

Fix menu buttons color

fc1c76a

issmail-basel requested review from HideBa, KaWaite and rot1024 August 24, 2021 07:10

rot1024 suggested changes Aug 24, 2021

View reviewed changes

src/components/molecules/Visualizer/Engine/index.tsx Outdated Show resolved Hide resolved

Basel-Issmail added 3 commits August 26, 2021 08:05

Reflect backend changes

d22a9a6

Get rid of context

d3aa42d

refactor code

68c9ea9

issmail-basel requested a review from rot1024 August 30, 2021 07:04

Add comment

b865df8

issmail-basel marked this pull request as ready for review August 30, 2021 07:15

Basel-Issmail added 2 commits August 30, 2021 10:18

Merge branch 'main' of github.com:reearth/reearth-web into scene-them…

1e95424

…e-front

Fix optional parameter

e3a81a9