-
Notifications
You must be signed in to change notification settings - Fork 615
moves widget operations to backend #5111
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
Conversation
…-widget-operations
…es skipEdit computed
03443c5 to
c1f79ee
Compare
| /> | ||
|
|
||
| <AposButton | ||
| v-if="widgetRemoveControl" |
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.
Still standalone, but might be removed (no reason to do that but) by users.
| import checkIfConditions from 'apostrophe/lib/universal/check-if-conditions.mjs'; | ||
| const standaloneWidgetOperation = [ 'remove' ]; |
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.
Not needed in data here, hard coded stuff.
| disabled, | ||
| tooltip: { | ||
| content: operation.label, | ||
| content: !disabled && (operation.tooltip || operation.label), |
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.
A bit weird to have label and tooltip for primary operations? But same config as secondary indeed. We have different label and tooltips for some primary operations which doesn't make sense (label not used).
| return controls.filter(control => ( | ||
| !this.widgetSkipControlsMap.get(control.action) && | ||
| !this.widgetSkipControlsMap.get(control.name) | ||
| )); |
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.
Not needed anymore
| !this.widgetSkipControlsMap.get(control.action) && | ||
| !this.widgetSkipControlsMap.get(control.name) | ||
| )); | ||
| return controls.concat(this.widgetCustomSecondaryOperations.map(renderOperation)); |
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.
use the same cb than for native, allow to disable custom secondary operations too.
| }, | ||
| action: 'remove' | ||
| } | ||
| }; |
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.
kept standalone, still takes its config from backend.
| return this.getOperations({ | ||
| secondaryLevel: true, | ||
| native: false | ||
| }); |
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.
We make difference between native and custom for secondary to show native first and to display a separator between both.
| (secondaryLevel && !operation.secondaryLevel) || | ||
| (!secondaryLevel && operation.secondaryLevel) | ||
| ) { | ||
| return false; |
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.
Separate filter logic to make it readable.
Also make the check first to avoid going inside checkIfConditions when operation is removed anyway.`
| }, | ||
| methods: { | ||
| getOperations({ secondaryLevel }) { | ||
| getOperations({ secondaryLevel, native }) { |
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 native approach should be enforced also in modules/@apostrophecms/area/ui/apos/components/AposBreadcrumbOperations.vue.
It would be tricky to not break stuff though. The context:
- currently if there is an
actionprop, it will be emitted on component level, otherwise the name becomes the action and is emitted via the message bus. - I'm not sure how this fits the
nativepart, as we are having a nonnativeaction operations. I'm open for any solutions.
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.
native is not really needed, it's more here because for widget operations we allowed only operations with a modal property. native operations have no modal properties..
But if breadcrumbs operations allow operations to emit action through the event bus it should be the same for widget operations no?
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.
Yes, it makes sense to me. Not currently needed, but consistent.
myovchev
left a comment
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.
Look at my comment - the breadcrumb operations need a look as well - they attempt to let the core actions work, but also allow emitting on component level vs message bus.
…disable tooltip on entire switch
| emits: [ 'switch-breakpoint-preview-mode', 'reset-breakpoint-preview-mode' ], | ||
| data() { | ||
| return { | ||
| mode: null, |
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.
get mode from store. It only contains mode and setMode for now. allows to get / watch / compute this value from other components.. Could be nice to move the entire logic in the store later, I kept it very simple for now.
| } | ||
| if (operation.action) { | ||
| apos.bus.$emit(operation.action, payload); |
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.
Not sure to understand but:
AposAreaLayoutEditorlistens to both events directly (from bus):
apos.bus.$on('apos-switch-layout-mode', this.switchLayoutMode);
apos.bus.$on('apos-layout-col-delete', this.onRemoveLayoutColumn);Also reworked WidgetOperations to use nativeAction.
| </label> | ||
| </div> | ||
| <span class="apos-breadcrumb-switch__input-text">{{ $t(choice.label) }}</span> | ||
| </label> |
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 mainly added classes, removed a div and handle disable states.
| 'hoveredNonForeignWidget', | ||
| 'emphasizedWidgets' | ||
| ]), | ||
| ...mapState(useBreakpointPreviewStore, { breakpointPreviewMode: 'mode' }), |
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.
Rename mode in breakpointPreviewMode for clarity.
| layoutWidgetGrid.tablet?.breakpoint, | ||
| layoutWidgetGrid.mobile?.breakpoint | ||
| ); | ||
| return screenWidth <= tinyScreenStart; |
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.
as asked we will disable the button as soon as the screen is smaller than tablet or mobile (bigger one)
| tooltip: operation.tooltip || null, | ||
| teleportContent: this.teleportModals | ||
| teleportContent: this.teleportModals, | ||
| disabled |
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.
Has been properly tested for switch only, will create a ticket.
| computed: { | ||
| isDisabled() { | ||
| return this.disabled || this.enhancedChoices.some((choice) => choice.disabled); | ||
| }, |
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 limited the breadcrumb switched to 2 choices only (server crashes otherwise with error). So we can use @stuartromanek style (everything is grayed if one choice is disabled or the entire switch is)
boutell
left a comment
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 one minor language thing
| }, | ||
| disabled: { | ||
| type: Boolean, | ||
| default: false |
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.
Not so far anyway right? We disable one of the choices but not the active one (edit content).
| operationsInBreadcrumb: true, | ||
| breakpoints: { | ||
| tablet: 1024, | ||
| mobile: 414 |
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.
Change back
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.
Done!
| type: 'boolean', | ||
| label: 'apostrophe:layoutTabletShow', | ||
| help: `Less than ${options?.breakpoints?.tablet || 900}px`, | ||
| help: `Less than ${options?.breakpoints?.tablet || 1024}px`, |
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 this default fallback is possible, right?
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.
Maybe if users override the config somehow?
Better would be forcing options to be present maybe.
| type: 'boolean', | ||
| label: 'apostrophe:layoutMobileShow', | ||
| help: `Less than ${options?.breakpoints?.mobile || 600}px`, | ||
| help: `Less than ${options?.breakpoints?.mobile || 414}px`, |
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 this default fallback is possible, right?
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.
Maybe if users override the config somehow?
| return; | ||
| } | ||
| this.$emit('update', null); |
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.
This watch just remove the selected choice if it becomes disabled (eg. edit columns becomes disables it will switch to content automatically)
| cursor: pointer; | ||
| letter-spacing: 0.5px; | ||
| border-radius: 4px; | ||
| } |
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.
This css is jsut about using classes and adding @stuartromanek style when disabled.
| } | ||
| }, | ||
| emits: [ 'remove', 'edit', 'cut', 'copy', 'clone', 'up', 'down', 'update' ], | ||
| emits: [ 'update', 'operation' ], |
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.
follows @myovchev logic, we emit operation with the right name and not the event itself.
|
@myovchev Fixes the native operations for breadcrumb too, move is working well. |
Summary
Pretty simple solution, no big refacto. Still moves all widget operations config backend (with specific frontend native logic).
Removes the need for
skipOperations.A bit easier to read since all operations come from the same config (less vue code).
What are the specific steps to test this change?
For example:
What kind of change does this PR introduce?
(Check at least one)
Make sure the PR fulfills these requirements:
If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.
Other information: