-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
feat: add base changelog list component #8
Conversation
redbeard0091
commented
Jul 23, 2024
•
edited
Loading
edited
- adds a new base changelog list component
- add overridable error and loading components
- improves documentation
🎉 This issue has been resolved in version 1.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
``` | ||
# npm | ||
npm run rev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm run rev | |
npm run dev |
} | ||
|
||
return ( | ||
<div> | ||
<ChangelogContext.Provider value={{ loading, error, data }}> | ||
{children} | ||
<ChangelogContext.Provider value={{ data }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ich würde den outer div entfernen. So forced to bei Benutzung eine Mutation im DOM
|
||
export const useUpdateHiveContext: () => UpdateHiveHookResult = () => { | ||
export const ChangelogContext = createContext<ChangelogContextProps>({}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wäre es sinnvoll data als empty array zu initilisieren?
<div> | ||
{componentChangelogs.map((changelog, index) => ( | ||
<div key={`changelog-${index}`}> | ||
<Box sx={() => ({ marginBottom: '8px' })}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Box sx={() => ({ marginBottom: '8px' })}> | |
<Box sx=({ mb: 2 }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auch die anderen mal anschauen. Es gibt shorthandler und wenn du das theme objeckt nicht nutzt kannst du die funktion omitten
changelogs: Changelog[]; | ||
} | ||
|
||
const SimpleList: React.FC<Props> = ({ changelogs, changeTypeMapper }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name würde ich überdenken. Gibt es auch eine "HardList"? "ComplicatedList"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mir ist auch nicht so ganz klar warum diese Komponente fast genauso wie ComponentList aussieht. Kann man vll etwas generisches aus beiden heraus lösen und diese hier mergen?