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

IBX-2925: [D&D] Functionality and design improvements #435

Merged
merged 23 commits into from
Nov 3, 2022

Conversation

GrabowskiM
Copy link
Contributor

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-2925
Bug fix? no
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@@ -120,6 +127,12 @@
group.reinit();
});
};
const insertFieldDefinitionNode = (fieldNode) => {
const fieldDefinitionNode = createFieldDefinitionNode(fieldNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const fieldDefinitionNode = createFieldDefinitionNode(fieldNode);
const fieldDefinitionNode = createFieldDefinitionNode(fieldNode);

insertFieldDefinitionNode(response);
afterChangeGroup();
})
.catch(ibexa.helpers.notification.showErrorNotification);
.catch((error) => {
const field = doc.querySelector('.ibexa-collapse--field-definition-loading');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const field = doc.querySelector('.ibexa-collapse--field-definition-loading');
const field = doc.querySelector('.ibexa-collapse--field-definition-loading');

const field = doc.querySelector('.ibexa-collapse--field-definition-loading');
setErrorField(field);
ibexa.helpers.notification.showErrorNotification(error);
});
};
const reorderFields = () => {
insertFieldDefinitionNode(currentDraggedItem);

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

const targetContainerGroup = targetContainer.closest('.ibexa-collapse--field-definitions-group');
const targetContainerList = targetContainerGroup.closest('.ibexa-content-type-edit__field-definitions-group-list');
const fieldTemplate = targetContainerList.dataset.template.replace('{{ type }}', currentDraggedItem.dataset.itemIdentifier);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const fieldTemplate = targetContainerList.dataset.template.replace('{{ type }}', currentDraggedItem.dataset.itemIdentifier);
const fieldTemplate = targetContainerList.dataset.template;
const fieldRendered = fieldTemplate.replace('{{ type }}', currentDraggedItem.dataset.itemIdentifier);

?

return false;
}

// 0 makes that item is always inserted before anchored placeholder
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// 0 makes that item is always inserted before anchored placeholder

@@ -432,6 +534,7 @@
draggable.init();
draggableGroups.push(draggable);
});
fieldDefinitionsGroups.forEach((group) => group.addEventListener('click', () => setActiveGroup(group)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fieldDefinitionsGroups.forEach((group) => group.addEventListener('click', () => setActiveGroup(group)));
fieldDefinitionsGroups.forEach((group) => group.addEventListener('click', () => setActiveGroup(group), false));

'is_expanded': false,
'is_draggable': true,
'class': 'ibexa-collapse--field-definition ibexa-collapse--field-definition-highlight ibexa-collapse--field-definition-loading',
'body_id': 'loading_collapse',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'body_id': 'loading_collapse',
'body_id': 'loading-collapse',

@GrabowskiM GrabowskiM marked this pull request as draft June 9, 2022 08:20
@GrabowskiM GrabowskiM force-pushed the IBX-2925-dnd-improvements branch 2 times, most recently from bc08e89 to a1e7f10 Compare September 16, 2022 10:23
@GrabowskiM GrabowskiM marked this pull request as ready for review September 16, 2022 10:26
@GrabowskiM GrabowskiM force-pushed the IBX-2925-dnd-improvements branch 2 times, most recently from 6bc76e4 to bf9ba83 Compare October 3, 2022 12:15
class="ibexa-available-field-type"
data-item-identifier="{{ item.identifier }}"
>
<div
Copy link
Contributor

Choose a reason for hiding this comment

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

indent?
and

  • is not draggable? how will it work, the li element will stay after drag?

  • Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    in this case, these are field types in right sidebar, this element always stay in its place, as you can add it multiple times

    @@ -28,7 +28,7 @@
    {% if extra_actions is defined %}
    {% for action in extra_actions %}
    <button type="button" class="ibexa-collapse__extra-action-button btn ibexa-btn ibexa-btn--no-text {{ action.button_class|default('') }}">
    <svg class="ibexa-icon ibexa-icon--small">
    <svg class="ibexa-icon {{ action.icon_size_class|default('ibexa-icon--small') }}">
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    A thought: icon_size_class seems to lack flexibility in case of future changes and new variables.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    Do you think it would be better to do

    <svg class="{{ action.icon_class|default('ibexa-icon ibexa-icon--small') }}">
    

    and later pass 'icon_class': 'ibexa-icon ibexa-icon--tiny-small', ? Or treat ibexa-icon as class that is always and only change icon_size_class to icon_class?

    }
    }

    &--is-highlighted {
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Whereas I understand that "action" classes like --is-dragging-out sound better with is, here and in other "non-action" classes below it is not needed in my opinion and it is not consistent with our other classes.

    Copy link
    Contributor

    @katarzynazawada katarzynazawada left a comment

    Choose a reason for hiding this comment

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

    There are some issues, I will list all on this PR

    1. Content Types

    • make d&d icon black when hover (in all d&d places)

    Screenshot 2022-10-18 at 11 18 01

    • when editing Content Type unnecessary grey background at the end

    Screenshot 2022-10-18 at 11 21 31

    2. Attributes in PC

    • blue shadow when hover attribute

    Screenshot 2022-10-18 at 11 02 12

    3. Perso
    Screenshot 2022-10-18 at 11 13 48

    • blue background, no animations

    4. Page Builder

    • no blue highlight after dropping element
    • no grey highlight after removing element, waiting for server response
    • no animation of removing element
    • issue when wrapping groups, see please: https://recordit.co/HcOlaG408y

    5. Form Builder

    Copy link
    Contributor

    @katarzynazawada katarzynazawada left a comment

    Choose a reason for hiding this comment

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

    There are still some issues.

    1. When adding two fields quickly(when blue highlight is visible) error occurs https://recordit.co/QJgTgqD0hW
    2. Blue indicator is not visible when adding a second or additional field https://recordit.co/sqDPluh9r0
    3. Cannot quickly remove field from Form Builder https://recordit.co/xsqN5rKESh
    4. Perso: missing blue indicator and remove animation https://recordit.co/zRRMgtVxqZ

    @dew326 dew326 merged commit 6f249aa into main Nov 3, 2022
    @dew326 dew326 deleted the IBX-2925-dnd-improvements branch November 3, 2022 12:18
    @sonarcloud
    Copy link

    sonarcloud bot commented Nov 3, 2022

    Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

    Bug A 0 Bugs
    Vulnerability A 0 Vulnerabilities
    Security Hotspot A 0 Security Hotspots
    Code Smell A 7 Code Smells

    No Coverage information No Coverage information
    0.0% 0.0% Duplication

    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.

    9 participants