-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[MBL-856] Banner For Creator Dashboard Deprecation #1830
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1830 +/- ##
==========================================
- Coverage 84.37% 84.33% -0.04%
==========================================
Files 1276 1277 +1
Lines 115816 115881 +65
Branches 30793 30810 +17
==========================================
+ Hits 97714 97730 +16
- Misses 17036 17084 +48
- Partials 1066 1067 +1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Kickstarter-iOS/Features/Dashboard/Controller/DashboardViewController.swift
Outdated
Show resolved
Hide resolved
Kickstarter-iOS/Features/Dashboard/Controller/DashboardViewController.swift
Outdated
Show resolved
Hide resolved
@msadoon Since this view is being added directly to the window, see my comment here, the DashboardViewController snapshots will still pass as is since the banner isn't technically being added to it. I think this is ok in this case because the feature will be removed in a month, but if you have ideas on how to capture this in our snapshot tests please lmk. I'm not sure how to reference the window itself, or if we even want to for this test case. |
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.
Ok, try to remember not to assign PR's that are in "draft".
I pushed a commit that basically is all the changes I worked on and tested. It uses the basic structure you have here, but cleaned up the code a lot.
There's a lot of changes here so let's go over them on a call, I think its' more efficient that way. You can also question any of the things I've done and I'll be happy to explain them.
The only thing I missed was covering the bottom scrollable area with the banner. I don't think its' super important, but if you think it is, we can work on that in the call together.
In the future let's start with an engineering plan so we're on the same page about the approach before coding.
Kickstarter-iOS/Features/Dashboard/Controller/DashboardViewController.swift
Show resolved
Hide resolved
Kickstarter-iOS/Features/Dashboard/Controller/DashboardViewController.swift
Outdated
Show resolved
Hide resolved
Kickstarter-iOS/Features/Dashboard/Controller/DashboardViewController.swift
Outdated
Show resolved
Hide resolved
Kickstarter-iOS/Features/DashboardDeprecationBanner/DashboardDeprecationView.swift
Show resolved
Hide resolved
Kickstarter-iOS/Features/DashboardDeprecationBanner/DashboardDeprecationView.swift
Show resolved
Hide resolved
Kickstarter-iOS/Features/DashboardDeprecationBanner/DashboardDeprecationView.swift
Outdated
Show resolved
Hide resolved
Kickstarter-iOS/Features/DashboardDeprecationBanner/DashboardDeprecationView.swift
Show resolved
Hide resolved
Kickstarter-iOS/Features/DashboardDeprecationBanner/DashboardDeprecationView.swift
Show resolved
Hide resolved
Kickstarter-iOS/Features/DashboardDeprecationBanner/DashboardDeprecationView.swift
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.
Tests not passing. Let's fix in pairing.
* makes sure the deprecation warning doesn't cover any table view content * moves code that provides a reference to the warning's UIHostingController into a helper method
… content inset update into a standalone func
Kickstarter-iOS/Features/Dashboard/Controller/DashboardViewController.swift
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.
Tests should pass - I did write a few commits because this PR was just taking too long.
So my advice here is to look at this commits and really understand what the diffs are between commits and why.
Again always happy to jump on a call and address any questions you have.
Otherwise I think this is good to go.
Like I said earlier let's start with an engineering plan first to avoid all this back and forth.
private func formatted(dateString: String) -> String { | ||
let date = self.toDate(dateString: dateString) | ||
return Format.date( | ||
secondsInUTC: date.timeIntervalSince1970, | ||
template: "MMMM d, yyyy", | ||
timeZone: UTCTimeZone | ||
) | ||
} | ||
|
||
private func toDate(dateString: String) -> Date { | ||
// Always use UTC timezone here this date should be timezone agnostic | ||
guard let date = Format.date( | ||
from: dateString, | ||
dateFormat: "yyyy-MM-dd", | ||
timeZone: UTCTimeZone | ||
) else { | ||
fatalError("Unable to parse date format") | ||
} | ||
|
||
return date | ||
} |
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.
So the reason tests were failing was because these functions were outside the struct
but declared as private
. They had the same name as the functions outside ManagePledgePaymentMethodsViewModel
which were also private
and outside the class
. So ManagePledgeViewControllerTests.testView_CurrentUser_IsBacker()
is the test that was breaking, it is no longer because these functions don't collide anymore.
let style = self | ||
.isTabBarItemLastItem(for: index) ? | ||
profileTabBarItemStyle(isLoggedIn: data.isLoggedIn, isMember: data.isMember) : | ||
dashboardTabBarItemStyle | ||
_ = tabBarItem(atIndex: index) ?|> style |
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.
I had to do some rework here to configure the logic around getting the tab bar styling, which was still breaking.
Basically the logic is now - if the app is styling the dashboard tab bar item, it should not be the last tab bar item. If it is, then style it as profile.
So the cases are:
- Logged in as backer (creator dashboard flag ON or OFF):
- The
setTabBarItemStyles
function never gets its' tab bar item case enum passed in asdashboard
, because the function that calls this one checks if the user is amember
first.
- Logged in as a creator (creator dashboard flag ON):
- If set in Beta tools as ON then go from backer --> creator, then the dashboard is visible.
- Turning this flag OFF in beta tools doesn't reflect the hiding of the dashboard until the user relaunches the app.
- Logged in as a creator (creator dashboard flag OFF):
- If set in Beta tools as OFF then go from backer --> creator, then the dashboard is not visible.
- Turning this flag ON in beta tools doesn't reflect the showing of the dashboard until the user relaunches the app.
Validate my logic here. I tested it, but if you can find a missing step I'm happy to rework this.
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.
Interesting that I didn't see this bug, but I've tested the current logic for all cases listed and the app is working as expected. Good to go 👍
private func updateTableViewBottomContentInset() { | ||
if let deprecationWarningHostingController = deprecationWarningHostingController { | ||
self.tableView.contentInset | ||
.bottom = deprecationWarningHostingController.view.bounds |
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.
Found that the depecrationWarningHostingController.view.bounds
always returns the height
and all other dimensions as .zero
if viewWillAppear
is called, so need to wait until viewDidAppear
is called for the views to have correct dimensions. That's why the self.tabBarController.tabBar.bounds.height
was needed - because it existed before viewWillAppear
on this view controller.
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.
nice
📲 What
A banner at bottom of dashboard notifying users of planned deprecation.
🤔 Why
We'd want to give users a heads up that features they may be used to seeing/using are going to be removed.
🛠 How
Important
removeFromSuperview
in viewWillDisappear so that it does not show anywhere else when switch tabs or presenting other screens.👀 See
Trello, screenshots, external resources?
Translations
✅ Acceptance criteria
⏰ TODO