Skip to content
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(deriv_ui): [MOBC-740] move UI related packages to deriv_ui #422

Merged
merged 9 commits into from
Jan 30, 2024

Conversation

behnam-deriv
Copy link
Contributor

@behnam-deriv behnam-deriv commented Jan 26, 2024

Clickup link: https://app.clickup.com/t/20696747/MOBC-740

This PR contains the following changes:

  • moving deriv_banner to deriv_ui
  • moving deriv_date_range_picker to deriv_ui
  • moving deriv_expandable_bottom_sheet to deriv_ui
  • creating example app for deriv_ui and including the example app of above packages in it
  • moving test cases of above packages to deriv_ui

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

Pre-launch Checklist (For PR creator)

As a creator of this PR:

  • ✍️ I have included clickup id and package/app_name in the PR title.
  • 👁️ I have gone through the code and removed any temporary changes (commented lines, prints, debug statements etc.).
  • ⚒️ I have fixed any errors/warnings shown by the analyzer/linter.
  • 📝 I have added documentation, comments and logging wherever required.
  • 🧪 I have added necessary tests for these changes.
  • 🔎 I have ensured all existing tests are passing.

Reviewers

@ahrar-deriv @sahani-deriv

Pre-launch Checklist (For Reviewers)

As a reviewer I ensure that:

  • ✴️ This PR follows the standard PR template.
  • 🪩 The information in this PR properly reflects the code changes.
  • 🧪 All the necessary tests for this PR's are passing.

Pre-launch Checklist (For QA)

  • 👌 It passes the acceptance criteria.

Pre-launch Checklist (For Maintainer)

  • [sahani] I make sure this PR fulfills its purpose.

Copy link
Contributor

Choose a reason for hiding this comment

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

this later needs to be moved to deriv_localizations right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@sahani-deriv
Copy link
Contributor

I suggest we have one example app for all the ui components. For example we might have a list of components name and when we click on it we can view the component or something like this. If we all agree, this might be a work for another sprint.

@ahrar-deriv
Copy link
Contributor

think it is best we re-arrange the structure as we now have multiple widgets in different categories. so the top folder should not be presentation/widgets. we should move all one level up and each category has its own presentation layer with widgets inside and core ....

@behnam-deriv
Copy link
Contributor Author

I suggest we have one example app for all the ui components. For example we might have a list of components name and when we click on it we can view the component or something like this. If we all agree, this might be a work for another sprint.

Yes I agree. Gradually this is what we want to achieve. to have a single example app to showcase all the UI components.

@behnam-deriv
Copy link
Contributor Author

think it is best we re-arrange the structure as we now have multiple widgets in different categories. so the top folder should not be presentation/widgets. we should move all one level up and each category has its own presentation layer with widgets inside and core ....

I created a component category to include compound widgets that are not as simple as a single flutter widgets. we can discuss it further to reach a clean folder structure in deriv_ui

Copy link
Contributor

Choose a reason for hiding this comment

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

how about moving these to docs folder and linking it in the main readme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest we keep it in the same folder as it will be easier for clients to find the relevant files of the README they are reading. for general docs we can move it to docs folder.

/// Extension for [BuildContext] to get localization.
extension ContextExtension on BuildContext {
/// Get localization.
DateRangeLocalizations? get localization => DateRangeLocalizations.of(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

i think later we should use one localization class for all the components.

@behnam-deriv behnam-deriv changed the title refactor: [MOBC-740] move UI related packages to deriv_ui feat: [MOBC-740] move UI related packages to deriv_ui Jan 30, 2024
@sahani-deriv sahani-deriv changed the title feat: [MOBC-740] move UI related packages to deriv_ui feat(deriv_ui) : [MOBC-740] move UI related packages to deriv_ui Jan 30, 2024
@sahani-deriv sahani-deriv changed the title feat(deriv_ui) : [MOBC-740] move UI related packages to deriv_ui feat(deriv_ui): [MOBC-740] move UI related packages to deriv_ui Jan 30, 2024
@sahani-deriv sahani-deriv merged commit a2fd5b9 into deriv-com:master Jan 30, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants