-
Notifications
You must be signed in to change notification settings - Fork 2
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
Updated timeline molecule and atom, added one more state as failure #141
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on enhancing the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
please make the changes requested
flutter/digit-ui-components/storybook/lib/widgets/atoms/timeline_stories.dart
Show resolved
Hide resolved
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.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (3)
flutter/digit-ui-components/storybook/lib/widgets/molecules/timeline_molecule_stories.dart (1)
Line range hint
1-107
: Overall improvements in story structure with room for enhanced contentThe changes to the
timelineMoleculeStories()
function have significantly improved the structure and focus of the TimelineMolecule stories:
- Reducing the number of story variants from 4 to 2 simplifies the storybook and makes it easier to maintain.
- The remaining stories now clearly demonstrate two main use cases: a basic timeline and a collapsible timeline, providing a good overview of the component's capabilities.
- The standardized step structure across both stories improves consistency and readability.
These changes align well with the PR objective of updating the timeline molecule and atom. However, to fully showcase the new capabilities, including the added failure state mentioned in the PR title, consider the following suggestions:
- Include an example of the new failure state in one of the stories to demonstrate this new capability.
- Enhance the diversity of the step states in both stories. Currently, they follow a pattern of completed, present, and future states. Adding examples of the failure state and mixing up the order would provide a more comprehensive demonstration.
- Consider adding a third story that showcases a more complex, real-world scenario. This could include a mix of all possible states, including the new failure state, and more diverse content for labels and descriptions.
Example of including a failure state:
TimelineStep( label: 'Document Verification', description: ['Document verification failed due to missing information'], state: TimelineStepState.failed, ),These enhancements would provide a more comprehensive showcase of the TimelineMolecule's capabilities, including the new features mentioned in the PR objectives.
flutter/digit-ui-components/digit_components/lib/widgets/molecules/digit_timeline_molecule.dart (1)
Line range hint
209-257
: Refactor thesortSteps
function for clarity and efficiencyThe current implementation of the
sortSteps
function is complex and may not efficiently sort steps while preserving the positions offailed
steps. Consider refactoring the function to improve clarity and ensure correct behavior.Here's an example of how you might simplify the function:
List<TimelineStep> sortSteps(List<TimelineStep> steps) { // Separate failed steps to preserve their original positions List<int> failedIndices = []; List<TimelineStep> nonFailedSteps = []; for (int i = 0; i < steps.length; i++) { if (steps[i].state == TimelineStepState.failed) { failedIndices.add(i); } else { nonFailedSteps.add(steps[i]); } } // Sort non-failed steps based on desired order nonFailedSteps.sort((a, b) => _stepOrder(a.state).compareTo(_stepOrder(b.state))); // Re-insert failed steps at their original positions for (int index in failedIndices) { nonFailedSteps.insert(index, steps[index]); } return nonFailedSteps; } int _stepOrder(TimelineStepState state) { switch (state) { case TimelineStepState.completed: return 1; case TimelineStepState.present: return 2; case TimelineStepState.future: return 3; default: return 4; } }This refactored function separates failed steps to maintain their original positions and sorts the other steps based on the defined order.
flutter/digit-ui-components/storybook/lib/widgets/atoms/timeline_stories.dart (1)
Line range hint
98-183
: Eliminate duplicated code in the 'Completed' storyThe 'Completed' story replicates much of the code from the 'Ongoing' story, including knobs and widget lists. This duplication can lead to inconsistencies and increased maintenance efforts.
Utilize the helper functions created for the 'Ongoing' story to refactor this story as well:
Use the Knobs Helper Functions
builder: (context) { final showAdditionalWidget = getShowAdditionalWidget(context); final showAdditionalCollapsibleWidget = getShowAdditionalCollapsibleWidget(context); return DigitTimeline( currentStep: TimelineStepState.completed, label: context.knobs.text(label: "label", initial: 'Completed state'), description: const ['18 / 02 / 2023'], additionalWidgets: buildAdditionalWidgets(showAdditionalWidget), additionalHideWidgets: buildAdditionalHideWidgets(showAdditionalCollapsibleWidget), ); }Ensure Reusable Widget Lists
The
buildAdditionalWidgets
andbuildAdditionalHideWidgets
functions can be reused here without modification.Refactoring in this manner reduces code redundancy and enhances consistency across your stories.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- flutter/digit-ui-components/digit_components/lib/enum/app_enums.dart (1 hunks)
- flutter/digit-ui-components/digit_components/lib/widgets/atoms/timeline.dart (4 hunks)
- flutter/digit-ui-components/digit_components/lib/widgets/molecules/digit_timeline_molecule.dart (4 hunks)
- flutter/digit-ui-components/storybook/lib/widgets/atoms/timeline_stories.dart (2 hunks)
- flutter/digit-ui-components/storybook/lib/widgets/molecules/timeline_molecule_stories.dart (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
flutter/digit-ui-components/digit_components/lib/enum/app_enums.dart (1)
68-68
: LGTM! Verify usage in related components.The addition of the
failed
state to theTimelineStepState
enum aligns with the PR objective and provides more flexibility in representing timeline steps. This change enhances the granularity of possible states, which can improve user feedback in timeline-related UI components.To ensure this change is properly integrated, please run the following script to identify components that use this enum and may need updates:
This script will help identify components that may need to be updated to handle the new
failed
state.✅ Verification successful
LGTM! Changes to
TimelineStepState
have been successfully integrated across relevant components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of TimelineStepState enum in the codebase # Search for TimelineStepState usage echo "Files using TimelineStepState:" rg --type dart "TimelineStepState" ./lib # Search for specific state usages echo "\nUsage of completed state:" rg --type dart "TimelineStepState.completed" ./lib echo "\nUsage of present state:" rg --type dart "TimelineStepState.present" ./lib echo "\nUsage of future state:" rg --type dart "TimelineStepState.future" ./lib # Note: The new 'failed' state won't have existing usages, # but developers should consider adding it where appropriateLength of output: 628
Script:
#!/bin/bash # Description: Find usages of TimelineStepState enum in the entire codebase # Search for TimelineStepState usage echo "Files using TimelineStepState:" rg --type dart "TimelineStepState" # Search for specific state usages echo "\nUsage of completed state:" rg --type dart "TimelineStepState\\.completed" echo "\nUsage of present state:" rg --type dart "TimelineStepState\\.present" echo "\nUsage of future state:" rg --type dart "TimelineStepState\\.future" # Note: The new 'failed' state won't have existing usages, # but developers should consider adding it where appropriateLength of output: 18775
flutter/digit-ui-components/digit_components/lib/widgets/atoms/timeline.dart (2)
40-40
: Verify proper implementation ofLocalizedState
By extending
LocalizedState<DigitTimeline>
, ensure that all localization features are correctly implemented and that any necessary localization context is properly provided to child widgets.Please confirm that:
- All text displayed by
_TimelineState
uses the localization context.- Any overridden methods from
LocalizedState
are properly implemented.
288-292
: Confirm localization keys exist for default textsWhen
viewDetailText
andhideDetailText
arenull
, default localized strings are used. Ensure that the localization keysi18.common.viewDetails
andi18.common.hideDetails
are defined in your localization files to avoid runtime errors.Please check that your localization files include these keys:
i18.common.viewDetails
i18.common.hideDetails
No description provided.