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

Better framing of "Application" & "Device" views #1853

Merged
merged 5 commits into from
Mar 21, 2023

Conversation

joepavitt
Copy link
Contributor

@joepavitt joepavitt commented Mar 16, 2023

Description

Inspired by #1852 I've decided to use that same header style across our Applications & Devices too, it offers better boundaries, and also targets some of the thinking in #1830 whereby it's now possible to go from a Device straight to it's attached Instance.

Application View:

Screenshot 2023-03-16 at 16 03 16

Device View:

Screenshot 2023-03-16 at 16 23 32

Checklist

@joepavitt joepavitt marked this pull request as ready for review March 16, 2023 16:24
@joepavitt joepavitt added the size:XS - 1 Sizing estimation point label Mar 16, 2023
@joepavitt
Copy link
Contributor Author

No direct issue for this, so assigning labels so that it shows on the Dev Board

@joepavitt joepavitt added this to the 1.6 milestone Mar 16, 2023
@joepavitt joepavitt added task A piece of work that isn't necessarily tied to a specific Epic or Story. area:frontend For any issues that require work in the frontend/UI labels Mar 16, 2023
@joepavitt joepavitt self-assigned this Mar 16, 2023
@joepavitt joepavitt requested a review from Pezmc March 20, 2023 07:13
@@ -44,7 +75,9 @@ import { Roles } from '@core/lib/roles'

// components
import NavItem from '@/components/NavItem'
import SectionTopMenu from '@/components/SectionTopMenu'
import InstanceStatusHeader from '@/components/InstanceStatusHeader'
Copy link
Contributor

@Pezmc Pezmc Mar 21, 2023

Choose a reason for hiding this comment

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

Perhaps better for a wider discussion, but I'd strongly prefer we start using relative paths.

The magic @ alias:

  • Locks us to webpack making it harder to swap to another tool
  • Prevents lots of tooling from working correctly, including VSCodes "open file" shortcuts.
  • Prevents static analysis (one of the key advantages to ES6 imports)
  • Doesn't autocomplete
  • Makes it harder to differentiate the scope of the component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps better for a wider discussion,

Worth opening thread in Slack or issue to capture the sweep required to remove. I prefer the cleanliness of the path to read, and find it a lot easier to find the files when I have an "absolute" path, but appreciate your points presented, and would be fine to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although isn't the "@" alias part of vitest rather than webpack?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like vitest had to be told about the webpack alias to be able to resolve files properly.

The aliases are set up in config/webpack.config.js

@joepavitt joepavitt merged commit cc51bcc into main Mar 21, 2023
@joepavitt joepavitt deleted the 1851-applications-table branch March 21, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:frontend For any issues that require work in the frontend/UI size:XS - 1 Sizing estimation point task A piece of work that isn't necessarily tied to a specific Epic or Story.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants