-
Notifications
You must be signed in to change notification settings - Fork 309
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
NAS-132667 / 25.04 / Come up with a reusable component for master-detail view #11096
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #11096 +/- ##
=======================================
Coverage 82.34% 82.34%
=======================================
Files 1643 1644 +1
Lines 57484 57530 +46
Branches 5930 5936 +6
=======================================
+ Hits 47333 47373 +40
- Misses 10151 10157 +6 ☔ View full report in Codecov by Sentry. |
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.
Looks nice and works well.
When I wrote about having a tag for a header, I meant the header in details view. The idea was to try to hide the whole mobile business, so that developer that consumes this component does not have to care about mobile back button.
E.g. something similar to this:
<ix-page-header header>
<ix-all-instances-header></ix-all-instances-header>
</ix-page-header>
<ix-master-detail-view
#masterDetailView="masterDetailViewContext"
>
<ix-instance-list master></ix-instance-list>
<ng-container detail>
@if (selectedInstance()) {
<div detailHeader>
<!-- detailHeader renders mobile-back-button and wires communication with masterDetailView -->
<!-- custom html is also possible for icons on Datasets page -->
{{ 'Details for {name}' | translate: { name: selectedInstance().name } }}
</div>
<ix-instance-details
[instance]="selectedInstance()"
></ix-instance-details>
}
</ng-container>
</ix-master-detail-view>
<ix-master-detail-view | ||
#masterDetailView="masterDetailViewContext" | ||
> | ||
<ix-page-header header> |
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 don't think page header needs to be part of this directive.
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.
ah, yeah, I got you.
Thanks, I'll fix that.
src/app/modules/master-detail-view/master-detail-view.component.html
Outdated
Show resolved
Hide resolved
This PR has been merged and conversations have been locked. |
Testing:
see ticket.
I used slots for presentational logic:
So in instances we need to define expected slots:
Additional fixes/improvements:
❗
-- fixed
[ixDetailsHeight]
directive. (I noticed it was jumpy. Visually it was pretty glitchy because scrollbar was added to the main wrapper container and after a fraction of a second it was removed because details container received height which instead put the scrollbar on the details container itself)-- Fixed Instances selection (previously we double clicked the table row to view instance details)