Skip to content

Add functional form for classNames#533

Closed
rhdeck wants to merge 15 commits into
fkhadra:masterfrom
rhdeck:master
Closed

Add functional form for classNames#533
rhdeck wants to merge 15 commits into
fkhadra:masterfrom
rhdeck:master

Conversation

@rhdeck
Copy link
Copy Markdown
Contributor

@rhdeck rhdeck commented Oct 10, 2020

Summary

Disconnect react-toastify from its SCSS classes by passing replacement className props as functions that contain context.

Problem

If one wants to use a utility class framework (Tailwind!)

  1. The default classes keep interfering.
  2. Contextual information (what kind of toast is this?) is not available

Solution

Three props get changed to a function - or - string API:

  1. Toast.className
  2. Toast.bodyClassName (and ToastContainer.toastClassName)
  3. Toast.progressBarClassName (and ToastContainer.progressBarClassName)

The string API works as it did before - class(es) added through this prop are mixed via cx with the toastify classes compiled from CSS

The functional API takes an arrow function (or useCallback return value) to set the string based on contextual information

(args: {type: 
  | 'info'
  | 'success'
  | 'warning'
  | 'error'
  | 'default'
  | 'dark'; defaultClassName: string; rtl: boolean;  position?: 
  | 'top-right'
  | 'top-center'
  | 'top-left'
  | 'bottom-right'
  | 'bottom-center'
  | 'bottom-left';}) => string

The ToastContainer.className also takes a functional form, though without the type prop:

(args: {defaultClassName: string; rtl: boolean;  position?: 
  | 'top-right'
  | 'top-center'
  | 'top-left'
  | 'bottom-right'
  | 'bottom-center'
  | 'bottom-left';}) => string

The functional API takes an arrow function (or useCallback return value) to set the string based on contextual information

(args: {defaultClassName: string; rtl: boolean;  position: 'top-right'
  | 'top-center'
  | 'top-left'
  | 'bottom-right'
  | 'bottom-center'
  | 'bottom-left';}) => string

This allows one to create code flows that determine which string to draw based on the type, and choose whether to mix in the default classes.

Example

A tailwind CSS/UI styled notification, without editiing SCSS.

import { ToastContainer } from "@react-toastify";
import "react-toastify/dist/ReactToastify.css";
const contextClass = {
  success: "bg-blue-600",
  error: "bg-red-600",
  info: "bg-gray-600",
  warning: "bg-orange-400",
  default: "bg-indigo-600",
  dark: "bg-white-600 font-gray-300",
};
const App:FC = ()=>{
  return (
   <Main />
   <ToastContainer
      toastClassName={({ type }) => contextClass[type || "default"] + 
        " flex p-1 min-h-10 rounded-md justify-between overflow-hidden cursor-pointer"
      }
      bodyClassName={() => "text-sm font-white font-med block p-3"}
      position="bottom-left"
      autoClose={3000}
    />
  )
}

Checklist:

  • Added tests for functional form
  • Implemented in production (distributed as @raydeck/react-toastify until this gets merged)
  • Doesn't break existing API (see tailwind sample above)

Next steps post-merge

DONE Most likely other context args should get pulled in for a more powerful API. RTL? Position?


Before submitting a pull request, please make sure the following is done:

  • 1. Fork the repository and create your branch from master.
  • 2. Run yarn in the repository root.
  • 3. If you've fixed a bug or added code that should be tested, add tests!
  • 4. Ensure the test suite passes (yarn test).
  • 5. Run yarn start to test your changes in the playground.
  • 6. Update the readme is needed
  • 7. Update the typescript definition is needed
  • 8. Format your code with prettier (yarn prettier-all).
  • 9. Make sure your code lints (yarn lint:fix).

Learn more about contributing here

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 10, 2020

Coverage Status

Coverage decreased (-0.01%) to 93.58% when pulling 6461ebe on rhdeck:master into 439c60a on fkhadra:master.

@fkhadra
Copy link
Copy Markdown
Owner

fkhadra commented Oct 15, 2020

Hey @rhdeck,

I'm currently testing your changes. I never used tailwind but I've always wanted to 😄. You just gave me a good excuse to do so.

Thank you for the quality of the PR, really appreciate that.

Copy link
Copy Markdown
Owner

@fkhadra fkhadra left a comment

Choose a reason for hiding this comment

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

Currently, we pass the type and the default classes. Shouldn't we pass the position as well, what do you think ?

Comment thread src/components/ToastContainer.tsx Outdated
Comment thread src/components/ToastContainer.tsx
Comment thread src/components/ProgressBar.tsx Outdated
Comment thread src/components/Toast.tsx Outdated
Comment thread src/components/Toast.tsx Outdated
Comment thread src/components/ToastContainer.tsx Outdated
Comment thread src/components/ToastPositioner.tsx Outdated
@rhdeck
Copy link
Copy Markdown
Contributor Author

rhdeck commented Oct 17, 2020

Implemented requested fixes, updated the PR overview to reflect it, and made two additional improvements:

  1. ToastContainer.className now has a more constrained argument list than Toast.className or ProgressBar.className (e.g. no type)
  2. ToastClassName.type now restricts type to the ToastType string union, which should further reduce errors by users.

Copy link
Copy Markdown
Owner

@fkhadra fkhadra left a comment

Choose a reason for hiding this comment

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

I'll merge it during the week end. Thanks for the work !

@fkhadra
Copy link
Copy Markdown
Owner

fkhadra commented Nov 6, 2020

Your PR landed in v6.1.0

@fkhadra fkhadra closed this Nov 6, 2020
@michael-woodward-spok
Copy link
Copy Markdown

I think the docs still need to be updated, but this has made styling react-toastify so much easier! 🚀

@fkhadra
Copy link
Copy Markdown
Owner

fkhadra commented Nov 7, 2020

Yeah indeed. I'll try to update it this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants