Feat/theming

[yurisldk]

Description

To achieve theme overwrite per component:

1. FlowbiteTheme interface in src/lib/components/Flowbite/Flow bytes Theme.ts refactored. It make code more readable and self-documented.
2. All components interfaces was moved from FlowbiteTheme.ts to their main component files.
3. Flowbite component hasn't been tested. Test cases added. We can be more confident when update/change logic.
4. mergeDepp in src/helpers/mergeDeep.ts mutated target object, it fixed. mergeDepp additionally makes deep copy of target object.
5. Flowbite updated to handle nested providers. Added test cases to make sure that components themes isolated.
6. FlowbiteThemeProvider wrapper added to handle theme prop without dark and usePreferences props. Just for keep things simple.

All these steps allow us to use Flowbite and FlowbiteThemeProvider to ovveride components theme in the follow way:

const Test = () => (
  <div className="flex flex-col align-center space-y-8">
    <Flowbite theme={{ theme: { avatar: { size: { md: 'w-6 h-6' } } } }}>
      <div className="flex space-x-2 items-end">
        <Avatar />
        <span className="dark:text-slate-300">Uses ovverrided MD size: 'w-6 h-6'</span>
      </div>

      <Flowbite theme={{ theme: { avatar: { size: { md: 'w-8 h-8' } } } }}>

        <Flowbite theme={{ theme: { avatar: { rounded: 'rounded-xl' } } }}>
          <div className="flex space-x-2 items-end">
            <Avatar rounded />
            <span className="dark:text-slate-300">Uses ovverrided MD size: 'w-8 h-8' from outer provider</span>
          </div>
        </Flowbite>

        <div className="flex space-x-2 items-end">
          <Avatar />
          <span className="dark:text-slate-300">Uses ovverrided MD size: 'w-8 h-8'</span>
        </div>

      </Flowbite>

    </Flowbite>

    <div className="flex space-x-2 items-end">
      <Avatar />
      <span className="dark:text-slate-300">Uses default MD size: 'w-10 h-10'</span>
    </div>
  </div>
);

or a bit cleaner with wrapper:

const Test = () => (
  <div className="flex flex-col align-center space-y-8">
    <FlowbiteThemeProvider theme={{ avatar: { size: { md: 'w-6 h-6' } } }}>
      <div className="flex space-x-2 items-end">
        <Avatar />
        <span className="dark:text-slate-300">Uses ovverrided MD size: 'w-6 h-6'</span>
      </div>

      <FlowbiteThemeProvider theme={{ avatar: { size: { md: 'w-8 h-8' } } }}>
        <FlowbiteThemeProvider theme={{ avatar: { rounded: 'rounded-xl' } }}>
          <div className="flex space-x-2 items-end">
            <Avatar rounded />
            <span className="dark:text-slate-300">Uses ovverrided MD size: 'w-8 h-8' from outer provider</span>
          </div>
        </FlowbiteThemeProvider>

        <div className="flex space-x-2 items-end">
          <Avatar />
          <span className="dark:text-slate-300">Uses ovverrided MD size: 'w-8 h-8'</span>
        </div>
      </FlowbiteThemeProvider>
    </FlowbiteThemeProvider>

    <div className="flex space-x-2 items-end">
      <Avatar />
      <span className="dark:text-slate-300">Uses default MD size: 'w-10 h-10'</span>
    </div>
  </div>
);

Fixes #443 #442

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Adds test cases src/lib/components/Flowbite/Flowbite.spec.tsx

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

[codecov]

Codecov Report

Base: 97.08% // Head: 98.86% // Increases project coverage by +1.77% 🎉

Coverage data is based on head (4de222a) compared to base (ae95b9b).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 4de222a differs from pull request most recent head 6e79ba8. Consider uploading reports for the commit 6e79ba8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #450      +/-   ##
==========================================
+ Coverage   97.08%   98.86%   +1.77%     
==========================================
  Files         127      102      -25     
  Lines        4741     4770      +29     
  Branches      433      402      -31     
==========================================
+ Hits         4603     4716     +113     
+ Misses        138       54      -84     

Impacted Files	Coverage Δ
src/lib/components/Accordion/Accordion.tsx	100.00% <100.00%> (ø)
src/lib/components/Alert/Alert.tsx	100.00% <100.00%> (ø)
src/lib/components/Avatar/Avatar.tsx	98.46% <100.00%> (+0.38%)	⬆️
src/lib/components/Avatar/AvatarGroup.tsx	68.18% <100.00%> (+7.07%)	⬆️
src/lib/components/Avatar/AvatarGroupCounter.tsx	70.83% <100.00%> (+5.83%)	⬆️
src/lib/components/Badge/Badge.tsx	100.00% <100.00%> (ø)
src/lib/components/Breadcrumb/Breadcrumb.tsx	100.00% <100.00%> (ø)
src/lib/components/Button/Button.tsx	100.00% <100.00%> (ø)
src/lib/components/Button/ButtonGroup.tsx	100.00% <100.00%> (ø)
src/lib/components/Button/index.ts	100.00% <100.00%> (ø)
... and 48 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[yurisldk]

@rluders ready for review. Thank you.

[rluders]

I'm not sure about this... The Flowbite component handles not just the component styling, but also, dark and light mode. I'm afraid that it will cause a lot of "hooks" and extra processing under the hood.

[rluders]

What do you think @tulup-conner?

[yurisldk]

I think that make sense, also we can create object and then pass it to props and this way seems clear

const theme = {
  avatar: {
    size: {
      md: 'w-6 h-6',
    },
  },
};

const Test = () => (
  <Flowbite theme={{ theme }}>
    <div />
  </Flowbite>
);

[rluders]

I'm not convinced that using Flowbite everywhere or FlowbiteThemeProvider is the best approach. But I could be totally wrong. The initial idea to pass to the components the theme= was from @tulup-conner, so... I really would like to get his opinion about it.

What are the pros and cons of both approaches? How many renderings or hooks do we get? How does it impact the performance? AFAIK, provider context could be quite expensive: check here and also here.

But I'm not a front-end expert. 😶‍🌫️

From a code perspective, I feel quite confused about wrapping every component with the context provider in order to pass the theme to it. OK, it could look nice if you have multiple components that you need to customize in a section, but... Still, feels weird to have an "application theme context" and then inside this context have another "theme context" to overwrite one, two, or more components themes.

But this is just my opinion, I would like to hear some pros and cons of both approaches, so we could take a solid decision here. I felt like this is a really important and nice feature either way, and it will help the flowbite-react community a lot.

[tulup-conner]

• I've only thought having theme-per-component because I know it would be useful; I haven't reasoned through it at all until now.

With that said, this

<Flowbite theme={..}>
  <Component props />
</Flowbite>

Is more verbose than

<Component props theme={..} />

Critically, the theme context solution also adds a level of nesting, which is just not ideal ergonomically IMO.

With theme-per-component, in situations where you would like to reuse a flowbite-react component with a specific custom theme, you just extract that line to its own component.

const ThemedNavbar: FC = function () {
  return (
    <Navbar props theme={..} />
  )
}

This is the same methodology encouraged by the underlying technology, Tailwind CSS, in situations where you find yourself writing long-winded classNames.

Like @rluders said, the existing design of this Flowbite component serves a few different purposes, including the global dark/light theme setting. We would need to change this implementation further to extract the theme to its own component and context, presumably. I can't imagine a situation where having multiple contexts with their own idea of the current theme setting is useful. But I would have to see if DarkThemeToggle works as usual on this branch, in which case, probably not a big deal.

• I think the decision to couple the FlowbiteTheme pieces to their actual components is beyond sensible. We should definitely do that, IMO.
• I agree we should update mergeDeep since JS is call-by-sharing. I misunderstood the behavior. Thanks, I just learned about this because of your PR!
• On performance, I think people generally have a habit of prematurely optimizing things. We would want to look for metrics to determine whether performance meaningfully degrades with the Flowbite context providers, and how many providers need to exist before degradation starts. Tangentially, the inline theme solution also has the benefit of not being React Context - just a prop.

[rluders]

• I think the decision to couple the FlowbiteTheme pieces to their actual components is beyond sensible. We should definitely do that, IMO.

+1 to this... I think that we should definitely do it, no matter if we decide to go for per-component theming or not, this is definitely a huge benefit and introduces a lot to maintainability and readability to the codebase.

• On performance, I think people generally have a habit of prematurely optimizing things. We would want to look for metrics to determine whether performance meaningfully degrades with the Flowbite context providers, and how many providers need to exist before degradation starts. Tangentially, the inline theme solution also has the benefit of not being React Context - just a prop.

I don't want to look for premature optimization here, I think that the point was that given its complexity, and from what I read using Context could be a little bit trickier to use.

I would definitely aim for simplicity, and use the theme property per component. It would give us the flexibility that we are looking for to introduce an easier and more solid way to customize flowbite-react components without introducing unnecessary verbosity and under-the-hood processes.

So, @sldk-yuri how about we first aim to make all components have their own theme interface?

[yurisldk]

Thank you for you opinions @rluders @tulup-conner. Nested context is a one of the ways that we can use, probably the simpliest in current situation, becaue we don't have to refactor/update all components with theme prop to aplly theme ovveriding, just use wrapper. But in the same case deep context nesting could be a potential problem in the future and probably it's over-solution right now. Also we may add this feature in the future if needed. I'm going to start with theme prop option.

About this PR, I'm going to revert branch to f45cecf because this commit remove nested providers feature c4e195c and save:

• interface decomposition 1acaae8,
• moves interfaces to their components 1ac4e68,
• test cases for Flowbite provider 95e02fb
• deep Merge fix f45cecf.

Should we merge this PR with these 4 commits and open new one for components theme prop or should I continue to work here?

[rluders]

@sldk-yuri agreed. Let's continue on this PR, there is no problem. Maybe you just need to rebase it and drop the commits that we don't want, and it will be fine.

[rluders]

@sldk-yuri BTW, nice tip here... you don't need to merge the main into the feat/theming, you could use just the rebase:

1. Add flowbite upstream

git remote add upstream https://github.com/themesberg/flowbite-react.git

2. From your branch sync it with upstream/main

git pull upstream main --rebase

3. Handle any rebase conflict - do your stuff and...
4. If you need to update your PR just use:

git push origin your/pr --force

It is easier, better, cleaner, and elegant. :)

[yurisldk]

@rluders @tulup-conner PR is updated and ready to merge. I also think we have to add theme feature to components one by one, probably one big PR isn't good case. IMHO separate PRs for each component allow to service code easier, check changes cleaner and test changes better. I also think that there are will be some discussion about how to implement theme prop for different components, we can use #443 discusion for that or create new one.

[rluders]

@rluders @tulup-conner PR is updated and ready to merge. I also think we have to add theme feature to components one by one, probably one big PR isn't good case. IMHO separate PRs for each component allow to service code easier, check changes cleaner and test changes better. I also think that there are will be some discussion about how to implement theme prop for different components, we can use #443 discusion for that or create new one.

Good, let's make #433 an initiative and split it into an issue per component, it would be easier to handle and it will create smaller PRs. I'll do it today ASAP and also merge this PR once all the issues get created, 'cause I need to list the ones that this PR closes so I don't need to create the issues for those.

[rluders]

Ideally, we will have to move the interfaces inside their own component folders.

[rluders]

I created the respective tasks for every single component. We are going to handle it in separated PR.