"+" UIBarButton item to add list #4491

2023-02-15T07:24:44Z

pravalgautam commented

2023-02-15 07:24:44 +00:00

(Migrated from github.com)

Implements a "+" from #4188

biodranik (Migrated from github.com) reviewed 2023-02-15 07:24:44 +00:00

biodranik (Migrated from github.com) reviewed 2023-02-15 10:40:58 +00:00

iphone/Maps/UI/PlacePage/Components/PlacePagePreviewViewController.swift

biodranik (Migrated from github.com) commented

2023-02-15 10:40:58 +00:00

Is it from another PR?

pravalgautam (Migrated from github.com) reviewed 2023-02-15 10:41:28 +00:00

iphone/Maps/UI/PlacePage/Components/PlacePagePreviewViewController.swift

pravalgautam (Migrated from github.com) commented

2023-02-15 10:41:27 +00:00

yes

pravalgautam (Migrated from github.com) reviewed 2023-02-15 11:03:24 +00:00

iphone/Maps/UI/PlacePage/Components/PlacePagePreviewViewController.swift

pravalgautam (Migrated from github.com) commented

2023-02-15 11:03:24 +00:00

the pr for create new list and + sign is
cbfc515a8c821f5042f8341dc5eb2b10ef095db5

the pr for create new list and + sign is cbfc515a8c821f5042f8341dc5eb2b10ef095db5

vng (Migrated from github.com) reviewed 2023-03-06 20:25:52 +00:00

vng (Migrated from github.com) left a comment

@biodranik
Next steps here? LGTM
@pravalgautam
Could you please merge commits into one and sign it (DCO)

@biodranik Next steps here? LGTM @pravalgautam Could you please merge commits into one and sign it (DCO)

biodranik (Migrated from github.com) requested changes 2023-03-06 22:55:24 +00:00

biodranik (Migrated from github.com) left a comment

Please learn about git rebase and clean up your commits/squash them into one commit. And sign it properly with DCO.

Please also pay attention to failing checks: organicmaps/organicmaps#4491/checks

Please learn about `git rebase` and clean up your commits/squash them into one commit. And sign it properly with DCO. Please also pay attention to failing checks: https://git.omaps.dev/organicmaps/organicmaps/pulls/4491/checks

iphone/Maps/Bookmarks/Categories/BMCView/BMCViewController.swift

biodranik (Migrated from github.com) commented

2023-03-06 22:49:58 +00:00

Why is it not a system add button? https://developer.apple.com/documentation/uikit/uibarbuttonsystemitem/uibarbuttonsystemitemadd
Is it possible to specify createNewCategory selector directly?

1. Why is it not a system add button? https://developer.apple.com/documentation/uikit/uibarbuttonsystemitem/uibarbuttonsystemitemadd 2. Is it possible to specify createNewCategory selector directly?

biodranik (Migrated from github.com) commented

2023-03-06 22:51:47 +00:00

Please do not ignore the current code style.


  @objc func addTapped() {
    createNewCategory()
  }

Please do not ignore the current code style. ```suggestion @objc func addTapped() { createNewCategory() } ```

iphone/Maps/UI/PlacePage/Components/PlacePagePreviewViewController.swift

biodranik (Migrated from github.com) commented

2023-03-06 22:53:07 +00:00

Why is this file deleted?

xcode/shaders/shaders.xcodeproj/project.pbxproj

biodranik (Migrated from github.com) commented

2023-03-06 22:52:50 +00:00

Why is this change necessary?

biodranik (Migrated from github.com) reviewed 2023-03-07 11:27:03 +00:00

biodranik (Migrated from github.com) left a comment

I've updated the PR description.

Did you test list deletion and OM restarting? Does the order of lists stay the same?

I've updated the PR description. Did you test list deletion and OM restarting? Does the order of lists stay the same?

iphone/Maps/Bookmarks/Categories/BMCView/BMCViewController.swift

					
				@ -38,0 +40,4 @@

				    navigationItem.rightBarButtonItem = addButton

				  }

				    @objc func addTapped() {

				    createNewCategory()

biodranik (Migrated from github.com) commented

2023-03-07 11:16:09 +00:00

Will this work? (you likely need to add @objc annotation to createNewCategory() method.

    navigationItem.rightBarButtonItem = UIBarButtonItem(barButtonSystemItem: .add, target: self, action: #selector(createNewCategory))

Will this work? (you likely need to add `@objc` annotation to createNewCategory() method. ```suggestion navigationItem.rightBarButtonItem = UIBarButtonItem(barButtonSystemItem: .add, target: self, action: #selector(createNewCategory)) ```

biodranik (Migrated from github.com) requested changes 2023-03-08 13:49:19 +00:00

biodranik (Migrated from github.com) left a comment

The PR looks messed up. Did you learn about and try to git rebase and git commit --amend?

Let's split this issue into two. Please leave only adding of a + button in this PR. And create a new PR for the sorting issue.

The PR looks messed up. Did you learn about and try to git rebase and git commit --amend? Let's split this issue into two. Please leave only adding of a + button in this PR. And create a new PR for the sorting issue.

biodranik (Migrated from github.com) requested changes 2023-03-09 13:42:54 +00:00

iphone/Maps/Bookmarks/Categories/BMCView/BMCViewController.swift

					
				@ -38,0 +40,4 @@

				    navigationItem.rightBarButtonItem = addButton

				  }

				    @objc func addTapped() {

				    createNewCategory()

biodranik (Migrated from github.com) commented

2023-03-09 13:41:56 +00:00

Did you test it?

iphone/Maps/Bookmarks/Categories/BMCViewModel/BMCDefaultViewModel.swift

biodranik (Migrated from github.com) commented

2023-03-09 13:42:48 +00:00

As I understood from your testing, there are some issues with this approach, right?

Let's remove these lines from this PR so it can be merged. And implement sorting in another PR.

As I understood from your testing, there are some issues with this approach, right? Let's remove these lines from this PR so it can be merged. And implement sorting in another PR.

biodranik (Migrated from github.com) reviewed 2023-03-10 06:39:10 +00:00

iphone/Maps/Bookmarks/Categories/BMCView/BMCViewController.swift

					
				@ -38,0 +40,4 @@

				    navigationItem.rightBarButtonItem = addButton

				  }

				    @objc func addTapped() {

				    createNewCategory()