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.

Sign up for GitHub

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

Jump to bottom

PAINTROID-490 : Search bar on Landing Page #47

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

PAINTROID-490 : Search bar on Landing Page #47

wants to merge 3 commits into from

Conversation

dmdbilal
Copy link

@dmdbilal dmdbilal commented Feb 8, 2024 •
edited
Loading

New Features and Enhancements

Screenshots

Screenshot 1 Screenshot 2 Screenshot 3

Checklist

  • Include the name of the Jira ticket in the PR’s title
  • Include a summary of the changes plus the relevant context
  • Choose the proper base branch (develop)
  • Confirm that the changes follow the project’s coding guidelines
  • Verify that the changes generate no compiler or linter warnings
  • Perform a self-review of the changes
  • Verify to commit no other files than the intentionally changed ones
  • Include reasonable and readable tests verifying the added or changed behavior
  • Confirm that new and existing unit tests pass locally
  • Check that the commits’ message style matches the project’s guideline
  • Stick to the project’s gitflow workflow
  • Verify that your changes do not have any conflicts with the base branch
  • After the PR, verify that all CI checks have passed
  • Post a message in the #paintroid Slack channel and ask for a code reviewer

@Lenkomotive
Copy link
Contributor

@dmdbilal Hey! Please also add some unit/widget/integration tests. Cheers :)

@dmdbilal
Copy link
Author

dmdbilal commented Feb 26, 2024 •
edited
Loading

I'm getting this error all of the sudden. I've tried by cloning the latest version of the project and ran it. There also I'm getting this same error again.

image

@juliajulie95
Copy link
Contributor

I'm getting this error all of the sudden. I've tried by cloning the latest version of the project and ran it. There also I'm getting this same error again.

image

@Lenkomotive Can you help with this? I think it was added in your last ticket?

@bhav-khurana
Copy link
Contributor

I think that's because the current version doesn't support extending the Path class as it has only factory constructors

@Lenkomotive
Copy link
Contributor

@bhav-khurana thats correct! @dmdbilal As @bhav-khurana pointed out, the implementation of Path was updated since the version we currently use (3.10.5). Please also downgrade your version too while working on the project. Upgrading to a newer stable version is planned and I believe there is already a ticket. Cheers :)

@dmdbilal dmdbilal closed this Feb 27, 2024
@dmdbilal dmdbilal reopened this Feb 27, 2024
@dmdbilal
Copy link
Author

@Lenkomotive Now it's running without any errors.

@dmdbilal
Copy link
Author

@Lenkomotive Added widget test.

@juliajulie95
Copy link
Contributor

Please add a link to the ticket in your PR description

@dmdbilal
Copy link
Author

@juliajulie95 Added the link to the ticket in the PR description.

msesko
msesko suggested changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@msesko msesko left a comment

Choose a reason for hiding this comment

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

A lot to change. Pls look at it. You cannot just copy code. This makes the whole project unmaintainable

packages/features/landing_page_screen/lib/src/landing_page.dart
actions: const [MainOverflowMenu()],
actions: [
IconButton(
icon: const Icon(Icons.search_rounded, color: Colors.white),
Copy link
Contributor

Choose a reason for hiding this comment

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

please use custom colors we have in light theme. i think: lightColorTheme.onSurface is white color

packages/features/search_bar_screen/lib/src/components/image_preview.dart
Copy link
Contributor

Choose a reason for hiding this comment

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

this is duplicated code? it already exists in landing page screen. you can move that code in the component library and use it in both screen packages

packages/features/search_bar_screen/lib/src/components/project_list_tile.dart
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicated code

packages/features/search_bar_screen/lib/src/components/project_overflow_menu.dart
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicated code

packages/features/search_bar_screen/lib/src/search_screen.dart
Future<List<Project>> _getProjects() async =>
database.projectDAO.getProjects();

Future<bool> _loadProject(IOHandler ioHandler, Project project) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicated functions

packages/features/search_bar_screen/lib/src/search_screen.dart
);
}

Future<void> _openProject(Project? project, IOHandler ioHandler) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicated functions

packages/features/search_bar_screen/lib/src/search_screen.dart
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this is pretty much all duplicated. you need to modularize it if it is used in multiple places

packages/features/search_bar_screen/pubspec.yaml
package_info_plus: ^4.0.1
filesize: ^2.0.1

# Internal packages
Copy link
Contributor

Choose a reason for hiding this comment

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

all these internal packages are needed?

packages/features/search_bar_screen/pubspec.yaml
riverpod_annotation: ^2.1.1
freezed_annotation: ^2.4.1

intl: ^0.18.0
Copy link
Contributor

Choose a reason for hiding this comment

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

this is definetly not needed right?

@juliajulie95 juliajulie95 marked this pull request as draft April 23, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msesko msesko msesko requested changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dmdbilal @Lenkomotive @juliajulie95 @bhav-khurana @msesko