-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
R&I Layout editing mods #2256
R&I Layout editing mods #2256
Conversation
- Mods to c-frame-edit__move element styling; - Added object name to frame title;
- Removed is-drilled-in classes; - Solidified styles for c-frame-edit* and fixed it so it no longer blocks interior selection; - Properly handle .has-complex-content; - TODO: display layout grid on s-selected and s-selected-parent;
- Grid visibility now working properly for nested layouts, but approach is brittle as hell; - Removed .l-layout__object container, not needed; - Fixed --no-frame hiding all nested __headers; - TODOs: - Better strategy needed for s-selected and s-selected-parent;
- Both Display and Flexible Layouts;
- Reorg of is-editing styles; - Show frame move bar on hover, regardless of s-selected; - Hide frame move bar when resizing; - TODO: figure out why move bar transition is only enabled when s-selected is applied to holder;
- Functionality now as desired; - Refinements to move bar display anim and timing; - TODOs: -- Code cleanup; -- Now seeing a problem where the drop target Flex layout remains visible after moving sub-objects between columns.
- Frame-related is-editing and s-selected styling CSS cleaned up and reorganized - styles moved out of _global.scss and into more relevant files; - Added `u-inspectable` to Flex Layout frame markup; - Border settings and margin refined so that frames while browsing don't add margin;
- Toolbar now with color bg for better tie-in with edit area visually; - Edit colors refined for Snow theme, WIP; - Tweak to c-click-icon styling
- New $editUI* and $editFrame* constants in espresso; - Refined coloring; - New mixin `grippy`; - TODOs: - Bring Flex Layout containers (frames, container resize) into style parity; - Cleanup / remove unused old constants; - Sync up other themes;
- Fix frame hover effects to only shrink frame contents for move bar when element is a Display Layout frame; - Flex Layout container grippys now use same style as Disp Layouts; - Style cleanup in Espresso; - TODO: more cleanup, bring other themes into parity;
- CSS cleanups; - Hover and selected styling;
- Brought back pushBack and pullForward functions to allow light and dark themes to be more similar; - Maelstrom and Snow themes brought into parity with Espresso;
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.
@charlesh88 Good improvements to sub-object editing! Please merge with topic-core-refactor to get the latest changes to the display layout.
src/ui/components/ObjectFrame.vue
Outdated
let objectType = this.openmct.types.get(this.domainObject.type); | ||
if (!objectType || !objectType.definition) { | ||
return; // TODO: return unknown icon type. | ||
} |
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.
Is there an unknown icon type to return here? If not, let's track it as a todo item in TCR.
src/ui/components/ObjectFrame.vue
Outdated
return; // TODO: return unknown icon type. | ||
} | ||
|
||
return objectType.definition.complexContent; |
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.
The code here is almost a duplicate of cssClass(). Can we define a method to get the object type definition instead of repeating the code?
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.
@dtailor90 to address.
if (selection.length === 0) { | ||
return; | ||
} | ||
|
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.
setSelection() should not be removed. There has been some updates to this file since this change was done. Please merge with topic-core-refactor to get the latest code.
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.
Merged latest TCR, this should be good.
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.
Just played around with this branch and sub-object editing is really slick now, I'm super excited about it 😄 This is awesome stuff.
A couple of comments and questions inline.
@@ -234,12 +230,17 @@ | |||
this.initSelectIndex = this.layoutItems.length - 1; | |||
}, | |||
trackItem(item) { | |||
if (!item.identifier) { |
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.
Just wondering why this check is necessary now?
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.
@akhenry Good question. I added this check as I was updating the file to remove the drilled-in logic. An item that has no identifier (i.e, drawing items) doesn't need to be tracked. It's more performant to return right away than checking for the type. I added this check to improve the code and be consistent with untrackItem().
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.
Got it, thanks
src/api/types/TypeRegistry.js
Outdated
* Check if type is included in complex types. | ||
* @private | ||
*/ | ||
TypeRegistry.prototype.isTypeComplex = function (key) { |
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 wonder if, for now, we should keep this contained to layouts? As I understand it, this is very layout specific?
I'm also thinking it should be opt-out. ie the list should be of 'simple' object types of which there are far fewer (basically summary widgets and clocks?)
Longer term we will probably need to add some API that allows views to give layouts hints on how to display them, but I don't really want to address that quite yet.
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.
Per discussion with @akhenry and @dtailor90:
- List of complex content objects to be opt-out, and maintained within the Layout plugin.
- @dtailor90 to address.
Changes look good to me. Thanks! |
Changes focused on improving editing in Layouts with an emphasis on sub-object editing.
READY FOR REVIEW
Changes
is-drilled-in
classes;.has-complex-content
;.l-layout__object
container, not needed;--no-frame
hiding downstream nested__header
elements;is-resizing
property to DisplayLayout.vue to detect when a user is actively resizing a Layout's frame, in order to prevent intermittent hovers from triggering;Author Checklist