GH-1975 Theme Framework & GH-1972 Palm Theme

[benstrumeyer]

• Have you followed the guidelines in CONTRIBUTING.md?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• Have you added an explanation of what your changes do?
• Does your submission pass tests?
• Did you lint your code prior to submission?

• DO NOT MERGE
• This PR is to receive feedback on the theme framework and styling of the palm theme only
• Theme files are stored locally as opposed to on the Account project which we do not want. A new PR will be opened afterwards that passes the css correctly

[wlycdgr]

Still looking at the CSS stuff, but here are my other comments in the meantime. Lemme know if you have any questions!

[wlycdgr]

This is a good use case for contexts, which allow you to make this kind of 'global' setting available to every component in the tree without manually passing the value down component by component. In fact, themes are the example used in the context React docs! - https://reactjs.org/docs/context.html

[benstrumeyer]

Used context for the first time! It's soo clean

[wlycdgr]

Destructure to clean up a lil
const { checked, handleClick } = props;, etc

[benstrumeyer]

Destructured!

[wlycdgr]

add checked

[benstrumeyer]

Added checked!

[wlycdgr]

Try to resolve this using the suggestions at https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/label-has-associated-control.md if you haven't yet

[benstrumeyer]

Definitely will try to resolve those before resorting to eslint, I think I mistakenly put that here since there are no labels

[wlycdgr]

buttons suggests that the items are button objects/components. Use a name that better reflects the nature of the (on/off bool) elements.

[benstrumeyer]

Changed to buttonState! Actually, removed altogether per Ethan's comment:

It looks like this is the only place that you're using the local state in this component. You can instead just pass the props to the <RadioButton /> component like this:

const { indexClicked, handleItemClick } = this.props
// ...code in between...
	<RadioButton
		checked={index === indexClicked}
		handleClick={() => handleItemClick(index)}
	/>

Then, you can remove all of the code from componentDidMount and RadioButtonGroup.handleClick and not have to worry about local state.

[wlycdgr]

Nice

[wlycdgr]

👍

[wlycdgr]

Let's do same for this.state while we're at it.

[benstrumeyer]

Destructured this.state as well

[wlycdgr]

Update to dark-blue-theme here too

[wlycdgr]

And let's be consistent with the naming -default-theme, palm-theme, leaf-theme, etc

[benstrumeyer]

Ah I wanted to do that. It's my understanding that these theme names rely on the account project for API calls and we don't want to update these for now, however we have a ticket in the backlog for it https://cliqztix.atlassian.net/browse/GH-1974

[wlycdgr]

Was gonna say remember to remove this but I guess this isn't getting merged so it doesn't matter! :)

[benstrumeyer]

Removed it anyway!

[wlycdgr]

Use $summary_text

[wlycdgr]

(That's prob my bad from before!)

[benstrumeyer]

Changed all text colors to $summary_text!

[wlycdgr]

Is this meant to be an "on top of everything else for sure" value? If so, use max int

[benstrumeyer]

Used max int!

[wlycdgr]

Factor the determination of whether we shouldGet out to a helper. this avoids using let and ensures that getTheme has a single responsibility that matches its name.

[fcjr]

This function should not have to be changed in the real PR. I think these changes were just made for testing the themes locally, is that correct @benstrumeyer?

[benstrumeyer]

Ah yea, I should have let you know that this was re-added for testing purposes! My bad Ilya! This function is staying as is.

[wlycdgr]

Is this to avoid an error if tabId is undefined? What happens if it is undefined and so we pass 0 but there is not tab with id 0?

[benstrumeyer]

This was also added for testing purposes

[wlycdgr]

How comes this duplicates themes/theme.scss instead of also using it as the template in the manner of palm-theme.scss?

[benstrumeyer]

Good idea! I will do it after the LEAF theme if that's okay with you

[wlycdgr]

I am poking around to see if we can use the preferred @use directive instead (https://sass-lang.com/documentation/at-rules/use). It's not working yet but we should look into it some more....or have you already tried and it didn't work?

[benstrumeyer]

Oh, I didn't even know @use existed. I tried replacing @import with @use inside the account project (where I moved this code) and it seems to work there!

[fcjr]

Ignore my last comment, the early returns makes this work 😬I'm dumb, but regardless, can you swap the order of these statements, switches read a bit clearer when the default case is the last one

[benstrumeyer]

Oh wow, I didn't know it worked like that. I'll always put default on the bottom then. Either way, good find