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

feat: SelectTheme component #1300

Merged
merged 6 commits into from
Apr 21, 2023
Merged

feat: SelectTheme component #1300

merged 6 commits into from
Apr 21, 2023

Conversation

josephaxisa
Copy link
Contributor

No description provided.

@josephaxisa josephaxisa requested review from bryans99, jkaster and zeckertG and removed request for bryans99, jkaster and zeckertG April 14, 2023 19:36
@josephaxisa josephaxisa force-pushed the jax/select-theme branch 2 times, most recently from f8cd652 to 9f32fa3 Compare April 14, 2023 19:41
@josephaxisa
Copy link
Contributor Author

Apologies for the multiple updates. Had a minor issue when committing.

zeckertG
zeckertG previously approved these changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@zeckertG zeckertG left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple of questions

packages/embed-components/src/Theme/SelectTheme.spec.tsx Show resolved Hide resolved
packages/embed-components/src/Theme/SelectTheme.tsx Outdated Show resolved Hide resolved
josephaxisa
josephaxisa commented Apr 14, 2023
View reviewed changes
packages/embed-components/package.json
"@looker/sdk-node": "^23.4.0",
"@testing-library/react": "^11.2.7",
"@looker/components-test-utils": "^1.5.27",
"react-redux": "^7.2.9",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed as a dev dependency because Provider is used in tests

packages/embed-components/src/Theme/SelectTheme.spec.tsx Show resolved Hide resolved
packages/embed-components/src/Theme/SelectTheme.tsx Outdated Show resolved Hide resolved
jkaster
jkaster reviewed Apr 15, 2023
View reviewed changes
Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

Looking good! I have minor questions.

packages/embed-components/src/Theme/state/sagas.spec.ts Outdated Show resolved Hide resolved
bryans99
bryans99 reviewed Apr 17, 2023
View reviewed changes
Copy link
Collaborator

@bryans99 bryans99 left a comment

Choose a reason for hiding this comment

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

Looks reasonable.

I've made some suggestions and perhaps an experiment

packages/embed-components/src/GlobalStore/sagas.ts
import type { InitFactoryAction } from './slice'

function* initSaga(action: PayloadAction<InitFactoryAction>) {
const { initFactorySuccessAction, setFailureAction } = factoryActions
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can do this once outside of the saga.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, if I remember correctly I tried this when writing this and it failed because factoryActions got destructured before the slice was built. I'll try again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I get this:

    TypeError: Cannot destructure property 'initAction' of '_slice.themeActions' as it is undefined.

      36 |
      37 | const {
    > 38 |   initAction,
         |   ^
      39 |   loadThemeDataAction,
      40 |   getThemesAction,
      41 |   getDefaultThemeAction

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not seeing this when I try it in GlobalStore

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not seeing it in theme sagas either

packages/embed-components/src/GlobalStore/sagas.ts
}

export function* saga() {
const { initFactoryAction } = factoryActions
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can extract the actions outside of the saga and just do it once.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not seeing your issue

packages/embed-components/src/GlobalStore/store.ts
export const store = createStore({
preloadedState: {
factory: defaultFactoryState,
themes: defaultThemesState,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to see an experiment were the store is created empty and the slices are added dynamically.

I suspect it wont work because of the deep combine issue causing a re-render but I'd like to see what happens.

If this works, this would allow the developer to only load the slices they need

packages/embed-components/src/Theme/SelectTheme.spec.tsx Show resolved Hide resolved
packages/embed-components/src/Theme/SelectTheme.spec.tsx Outdated Show resolved Hide resolved
packages/embed-components/src/Theme/SelectTheme.tsx Outdated Show resolved Hide resolved
packages/embed-components/src/Theme/SelectTheme.tsx Outdated Show resolved Hide resolved
packages/embed-components/src/Theme/state/sagas.ts Show resolved Hide resolved
packages/embed-components/src/Theme/state/sagas.ts Show resolved Hide resolved
packages/embed-components/src/Theme/state/slice.ts Outdated Show resolved Hide resolved
josephaxisa
josephaxisa commented Apr 18, 2023
View reviewed changes
packages/embed-components/src/Theme/state/sagas.ts Outdated
Comment on lines 112 to 115
if (service.expired()) {
yield* call(getThemesSaga)
yield* call(getDefaultThemeSaga)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bryans99 are you ok with me reusing the other two sagas from this saga?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not put the action that triggers it. This will block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I could do that

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 changed it to call the actions (not put, as I want it to block).

jkaster
jkaster previously approved these changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

LGTM

packages/embed-components/src/Theme/SelectTheme.tsx Outdated Show resolved Hide resolved
zeckertG
zeckertG previously approved these changes Apr 19, 2023
View reviewed changes
@josephaxisa josephaxisa dismissed stale reviews from zeckertG and jkaster via 5486cef April 20, 2023 21:38
zeckertG
zeckertG previously approved these changes Apr 21, 2023
View reviewed changes
Copy link
Contributor

@zeckertG zeckertG left a comment

Choose a reason for hiding this comment

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

lgtm

jkaster
jkaster approved these changes Apr 21, 2023
View reviewed changes
Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

LGTM

@josephaxisa josephaxisa merged commit 95c0da6 into main Apr 21, 2023
@josephaxisa josephaxisa deleted the jax/select-theme branch April 21, 2023 21:57
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkaster jkaster jkaster approved these changes

@zeckertG zeckertG zeckertG approved these changes

@bryans99 bryans99 Awaiting requested review from bryans99

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

Successfully merging this pull request may close these issues.
4 participants
@josephaxisa @bryans99 @jkaster @zeckertG