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

Compact applications view #4079

Merged
merged 40 commits into from
Jul 3, 2024
Merged

Compact applications view #4079

merged 40 commits into from
Jul 3, 2024

Conversation

cstns
Copy link
Contributor

@cstns cstns commented Jul 1, 2024

Description

Alter the applications view to make it more compact

  • extracted instance actions and device actions into distinct mixins for re-usability
  • clear delete instance input before show to allow multiple instance deletions without the name persisting
  • altered editorLink and dashboardLink components to host default slots that can be overridden (in order to display plain icons instead of icons and text required for the compact view)
  • broke down the application view into smaller manageable components

Did not delete the previous application view styling & components. The previous components are interchangeable with the new ones, one could easily add a switch on top of the application view to toggle between the compact and wide application views.

We currently do not have or I couldn't find a device status component that would render icons instead of the status text so I defaulted to the existing device status pill for the time being. This can be sorted out in a follow up task.

Related Issue(s)

closes #4015

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

@cstns cstns self-assigned this Jul 1, 2024
@cstns cstns marked this pull request as draft July 1, 2024 08:44
Copy link

codecov bot commented Jul 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.75%. Comparing base (698d94c) to head (5b221c2).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4079   +/-   ##
=======================================
  Coverage   78.75%   78.75%           
=======================================
  Files         285      285           
  Lines       13045    13045           
  Branches     2907     2907           
=======================================
  Hits        10273    10273           
  Misses       2772     2772           
Flag Coverage Δ
backend 78.75% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cstns
Copy link
Contributor Author

cstns commented Jul 1, 2024

I ran into something strange and couldn't figure out why e2e tests are passing locally but failing in CI.

I finally got to replicate the CI result by running the npm run build command before running the test suite as opposed to running npm run serve locally.

@joepavitt
Copy link
Contributor

For the "X More" button too, I think it would look better as justify-content: space-between, with the chevron on the right-side

@joepavitt
Copy link
Contributor

joepavitt commented Jul 3, 2024

Also also, found it odd to look at and think it's because we're missing the "Instances" (or devices when appropriate) headers even when we don't have any in an application, adding it in feels a little cleaner/more consistent, and I think it makes the "The application has no..." message:

Screenshot 2024-07-03 at 13 37 15

@cstns
Copy link
Contributor Author

cstns commented Jul 3, 2024

CSS for the application panel seems to have broken as there is no bottom-left or bottom-right border on the cells

I'm not sure what I'm supposed to be looking at, I can't see the difference between the image you provided, the pr and what's on production

The white background for the Application name and summary boxes has changed to grey-300, rather than the white in the existing (and requested) designs.

Didn't notice that, I fixed it

Why include the label for the device status, when the design doesn't show it, and the instances don't have it? I know we have more room to play with, but it adds inconsistency in appearance and looks odd

It was a quick fix to the fact that we don't have icons for all statuses a device can find itself in

For the "X More" button too, I think it would look better as justify-content: space-between, with the chevron on the right-side

done

Also also, found it odd to look at and think it's because we're missing the "Instances" (or devices when appropriate) headers even when we don't have any in an application, adding it in feels a little cleaner/more consistent, and I think it makes the "The application has no..." message:

done

Scope Creep

Am doing my best to keep it under control. The biggest changes were the extraction of device/instance functionality into mixins so I won't repeat existing code and splitting the massive application component into smaller components

@joepavitt
Copy link
Contributor

I'm not sure what I'm supposed to be looking at, I can't see the difference between the image you provided, the pr and what's on production

On production, and in my design:

Screenshot 2024-07-03 at 15 50 24

@joepavitt
Copy link
Contributor

Just made some minor CSS amendments to clean up the structure a little:

  • Consistent padding around the status pill
  • remove ml-1 element that was pushing the icon in the status pill off-centre

Before

Screenshot 2024-07-03 at 16 03 56

After

Screenshot 2024-07-03 at 16 02 13

@joepavitt
Copy link
Contributor

Have also just added link :hover behaviour for the "More" and instance/device tiles too

@joepavitt joepavitt self-requested a review July 3, 2024 16:44
@joepavitt joepavitt merged commit a59a144 into main Jul 3, 2024
14 checks passed
@joepavitt joepavitt deleted the 4015_compact-applications-view branch July 3, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Compact" Applications view
3 participants