Errors: Banner message for error loading data on My Store dashboard #4704
Conversation
…ding or removing error banner
…es without pull to refresh
rachelmcr
added
type: enhancement
|
You can trigger an installable build for these changes by visiting CircleCI here. |
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
| /// | ||
| private lazy var innerStackView: UIStackView = { | ||
| let view = UIStackView() | ||
| view.layoutMargins = UIEdgeInsets(top: 0, left: Constants.leadingMargin, bottom: 0, right: 0) |
There was a problem hiding this comment.
I think it'd better if there's margin for the right inset too - this could cause missing margin for users using right-to-left languages like Arabic:
There was a problem hiding this comment.
Good catch, thanks! I added a margin for the right inset in cd48e49. I thought about using directionalLayoutMargins to add the inset to the leading edge specifically (not left/right) but I realized it would be good to have insets on both sides anyway in case of very long store names.
| }() | ||
|
|
||
| /// A stack view for views displayed between the navigation bar and content (e.g. store name subtitle, top banner) | ||
| /// | ||
| private lazy var stackView: UIStackView = { |
There was a problem hiding this comment.
Do you think it might be necessary to update the name of this property to mainStackView or headerStackView to make it clearer?
| }) | ||
| }() | ||
|
|
||
| private lazy var spacerView: UIView = { |
There was a problem hiding this comment.
I think a comment for this property can be helpful to understand its purpose - WDYT?
| private lazy var stackView: UIStackView = { | ||
| let view = UIStackView() | ||
| view.translatesAutoresizingMaskIntoConstraints = false | ||
| view.backgroundColor = .listForeground | ||
| view.layoutMargins = UIEdgeInsets(top: 0, left: navigationController?.navigationBar.directionalLayoutMargins.leading ?? 0, bottom: 0, right: 0) |
There was a problem hiding this comment.
This line seems to have caused an issue with the margin when app is launched in landscape mode - removing this seems to have fixed the issue. Bravo 🎉
| /// | ||
| func hideTopBannerView() { | ||
| topBannerView.removeFromSuperview() | ||
| spacerView.removeFromSuperview() |
There was a problem hiding this comment.
I think there's no need for adding and removing topBannerView and spacerView when showing / hiding the banner view. The great thing about stack view is that it can collapse or expand when showing / hiding its subviews.
Therefore, I'd suggest that you add these as arranged subviews right after adding the innerStackView, and hide them by default. When showing or hiding the banner view, all you need to do is show / hide these subviews and it should still work as expected.
There was a problem hiding this comment.
This is a great approach that I didn't think of, thanks! In 92e6c6c I changed how these views are configured, and it simplifies the way the constraints are set, too. Now the header stack view is configured up front with all the arranged subviews added, and as you suggested the methods to show or hide the banner just show/hide those subviews.
| @@ -9,7 +9,7 @@ protocol DashboardUI: UIViewController { | |||
| var scrollDelegate: DashboardUIScrollDelegate? { get set } | |||
|
|
|||
| /// Called when the Dashboard should display syncing error | |||
| var displaySyncingErrorNotice: () -> Void { get set } | |||
| var displaySyncingError: () -> Void { get set } | |||
There was a problem hiding this comment.
Thanks for renaming this too! 💯
|
Thanks for the review and suggestions, @itsmeichigo! This is ready for another look when you have a chance. |
Resolves: #4382
Description
To improve our error states, we are introducing a top banner that can be used whenever there is a problem loading data. The banner is collapsed by default and on select, users can read our troubleshooting tips or contact support directly.
This PR sets up the banner and adds it to the top of the My Store screen. (This banner replaces the "Unable to load content" notice we displayed before.)
Changes
DashboardViewController. A quick overview of the changes there:shouldShowStoreNameAsSubtitleistrue. The store name is now embedded inside another stack view, to retain its leading margin. (That margin is now defined with a constant equivalent to the margins it had before.)shouldShowStoreNameAsSubtitleisfalse, the stack view is added when the banner is displayed, with all necessary constraints. (Those constraints are otherwise set when the subtitle is configured and the dashboard UI is added below it).Testing
Submitter Checklist
Update release notes:
RELEASE-NOTES.txtif necessary.