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.

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

Add Expanded view for Time List #7378

Merged
merged 79 commits into from
Jan 31, 2024
Merged

Add Expanded view for Time List #7378

merged 79 commits into from
Jan 31, 2024

Conversation

shefalijoshi
Copy link
Contributor

@shefalijoshi shefalijoshi commented Jan 17, 2024

Closes #7377

Describe your changes:

Adds a new component that displays the timelist view in compact mode provided an option isCompact: true
Adds inspector configuration to set compact view enabled/disabled

TODO:

  • @charlesh88 and @shefalijoshi Need to figure out some details of the item color dot, countdown label, start/end times and progress pie.
  • @shefalijoshi Currently hardcoded time Relation and itemState css - this needs to be updated from the plan after merging Activity state display for plans in Gantt and Time list views #7370
  • @charlesh88 CSS final mile.
  • @charlesh88 and @shefalijoshi Wire up time hero context strings (STARTS, ENDS, etc.).
  • @charlesh88 and @shefalijoshi Finish wiring up time vs. status evaluations, like Incomplete and Overdue. This is needed to control the proper display of icons and the progress pie element.
  • @shefalijoshi Clicking an Activity should select the "Activity" tab in the Inspector.
  • Activities should show "STARTS/ENDS/ENDED" depending on their time - inferred states (OVERDUE/RUNNING LONG) should only show if the Activity is not started or in progress, respectively. 1/30/24: "OVERDUE" is not displaying for current Activities that are not started.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Has this been smoke tested?
  • Have you associated this PR with a type: label? Note: this is not necessarily the same as the original issue.
  • Have you associated a milestone with this PR? Note: leave blank if unsure.
  • Is this a breaking change to be called out in the release notes?
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 104 lines in your changes are missing coverage. Please review.

Comparison is base (d42aa54) 55.74% compared to head (5cfa222) 55.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7378      +/-   ##
==========================================
- Coverage   55.74%   55.45%   -0.30%     
==========================================
  Files         668      668              
  Lines       26723    26800      +77     
  Branches     2585     2606      +21     
==========================================
- Hits        14897    14862      -35     
- Misses      11115    11222     +107     
- Partials      711      716       +5     
Flag Coverage Δ
e2e-full 41.27% <ø> (-7.74%) ⬇️
e2e-stable 59.09% <100.00%> (-0.21%) ⬇️
unit 48.58% <32.02%> (-0.12%) ⬇️
Files Coverage Δ
...s/plan/inspector/components/PlanActivitiesView.vue 1.23% <ø> (ø)
src/plugins/timelist/constants.js 100.00% <100.00%> (ø)
...imelist/inspector/TimeListInspectorViewProvider.js 100.00% <ø> (+33.33%) ⬆️
src/plugins/utcTimeSystem/UTCTimeFormat.js 86.66% <ø> (ø)
...an/inspector/components/PlanActivityStatusView.vue 7.69% <0.00%> (ø)
.../plugins/timelist/inspector/FilteringComponent.vue 0.00% <0.00%> (ø)
src/plugins/timelist/inspector/EventProperties.vue 5.88% <0.00%> (ø)
...gins/timelist/inspector/TimelistPropertiesView.vue 7.69% <0.00%> (-1.40%) ⬇️
src/plugins/timelist/TimelistComponent.vue 56.81% <54.43%> (-2.38%) ⬇️
src/plugins/timelist/ExpandedViewItem.vue 5.00% <5.00%> (ø)

... and 11 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d42aa54...5cfa222. Read the comment docs.

@ozyx ozyx self-requested a review January 18, 2024 00:12
@charlesh88 charlesh88 changed the title Add compact view for timelist Add Expanded view for Time List Jan 18, 2024
@charlesh88
Copy link
Contributor

There was a bit of confusion on naming: 'Compact' should refer to the current Time List view:
image

'Expanded' is the name for the new style:
image

Copy link
Member

@ozyx ozyx left a comment

Choose a reason for hiding this comment

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

Reviewed and have some comments regarding the structure of the Vue components and some anti-patterns that we ought not to proliferate due to major performance issues-- we can nip this in the bud straight away with a bit of restructuring.

This also needs tests

src/plugins/timelist/CompactView.vue Outdated Show resolved Hide resolved
src/plugins/timelist/inspector/TimelistPropertiesView.vue Outdated Show resolved Hide resolved
src/plugins/timelist/inspector/TimelistPropertiesView.vue Outdated Show resolved Hide resolved
src/plugins/timelist/CompactViewItem.vue Outdated Show resolved Hide resolved
- Markup and CSS sanding and polishing.
- Still WIP!
charlesh88 and others added 6 commits January 19, 2024 13:37
- Label formatting Todo notes about states.
- Computed values and `v-ifs` added to control display for progress pie and countdown 'hero'.
- Still WIP!
- Add svg icons and some stubbed in logic.
- Still WIP!
Copy link
Member

@ozyx ozyx left a comment

Choose a reason for hiding this comment

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

Alright this is looking good! I've left a bunch of suggestions, mostly small stuff-- but we really, really ought to break those item objects up into individual properties or we're losing out on a lot of free perf benefits.

src/plugins/timelist/ExpandedViewItem.vue Outdated Show resolved Hide resolved
src/plugins/timelist/ExpandedViewItem.vue Outdated Show resolved Hide resolved
src/plugins/timelist/ExpandedViewItem.vue Show resolved Hide resolved
Comment on lines +257 to +269
setTimeContext() {
this.stopFollowingTimeContext();
this.timeContext = this.openmct.time.getContextForView(this.path);
this.followTimeContext();
},
followTimeContext() {
this.timeContext.on(TIME_CONTEXT_EVENTS.tick, this.updateTimestamp);
},
stopFollowingTimeContext() {
if (this.timeContext) {
this.timeContext.off(TIME_CONTEXT_EVENTS.tick, this.updateTimestamp);
}
},
Copy link
Member

Choose a reason for hiding this comment

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

TimeContext stuff is gonna be a great candidate for a Open MCT specific Composable :D

src/plugins/timelist/TimelistComponent.vue Show resolved Hide resolved
src/plugins/timelist/TimelistComponent.vue Outdated Show resolved Hide resolved
src/plugins/timelist/TimelistComponent.vue Show resolved Hide resolved
src/plugins/timelist/TimelistComponent.vue Outdated Show resolved Hide resolved
src/plugins/timelist/inspector/svg-progress.js Outdated Show resolved Hide resolved
src/plugins/timelist/TimelistComponent.vue Outdated Show resolved Hide resolved
src/plugins/timelist/TimelistComponent.vue Outdated Show resolved Hide resolved
src/plugins/timelist/ExpandedViewItem.vue Outdated Show resolved Hide resolved
Copy link
Member

@ozyx ozyx left a comment

Choose a reason for hiding this comment

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

Good stuff. Awesome job fixing the re-renders-- initial testing shows the list is much more performant 👏

This all LGTM. I have a few non-blocking comments/questions. nothing urgent:)

Comment on lines +125 to +147
name: {
type: String,
default: ''
},
start: {
type: Number,
default: 0
},
end: {
type: Number,
default: 0
},
duration: {
type: Number,
default: 0
},
countdown: {
type: Number,
default: 0
},
cssClass: {
type: String,
default: ''
Copy link
Member

Choose a reason for hiding this comment

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

This is great! Do you think name, start, end, duration, and countdown, should be required props?

default() {
return 'notStarted';
}
default: 'notStarted'
Copy link
Member

Choose a reason for hiding this comment

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

nice

let formattedValue;
if (itemProperty.format) {
const itemStartDate = new Date(value).toDateString();
const timestampDate = new Date(this.timestamp).toDateString();
formattedValue = itemProperty.format(value, this.item, itemProperty.key, this.openmct, {
formattedValue = itemProperty.format(value, undefined, itemProperty.key, this.openmct, {
Copy link
Member

Choose a reason for hiding this comment

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

oh weird. where's this defined?

Copy link
Contributor

@charlesh88 charlesh88 left a comment

Choose a reason for hiding this comment

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

Much better! Previous issues I commented on look to have been fixed.

  • "Cancelled” should be removed, and "Skipped" should be brought back.
  • The time context hero text (STARTS/ENDS/etc.) isn't reactive, but needs to be.

Copy link
Contributor

@charlesh88 charlesh88 left a comment

Choose a reason for hiding this comment

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

We still need to fix the Activity tab issue, but that is being tracked with #7442.

@akhenry akhenry added the pr:e2e:couchdb npm run test:e2e:couchdb label Jan 31, 2024
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Jan 31, 2024
@ozyx ozyx merged commit 18e4b9d into master Jan 31, 2024
36 checks passed
@ozyx ozyx deleted the timelist-compact-view branch January 31, 2024 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature Feature. Required intentional design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Expanded View" for timelist for Situational Awareness dashboards
5 participants