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

Add inplace edit for timeline name #12000

Merged
merged 2 commits into from May 16, 2018

Conversation

Projects
None yet
6 participants
@deb1990
Copy link
Contributor

commented Apr 19, 2018

Overview

This PR adds the following features to the New Case Type page.

  1. Added ability to edit timeline names inline.
  2. Removed the Advanced Section, because now names can be edited from the tabs directly.
  3. Removed the "Remove" label from the trash icon, because otherwise it was taking lot of space.

Before

before

After

after

Technical Details

  1. ang/crmCaseType/activitySetDetails.html file is removed, as we dont need the advanced section anymore.

  2. In ang/crmCaseType/edit.html Remove label is removed from trash icon.

  3. crmEditableTabTitle directive has been created to make the tab title editable also it supports

  • cancel edit if ESCAPE is pressed
  • cancel edit if input looses focus
  • cancel if Cross button is pressed
  • save if ENTER is pressed
  • save if Check button is pressed
@reneolivo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

Reviewing this PR

@@ -32,6 +34,14 @@
width: 10em;
}

.crmCaseType-edit-actset {

This comment has been minimized.

Copy link
@reneolivo

reneolivo Apr 19, 2018

Contributor

Please use .crmCaseType__activitySet-label--edit:

  • no need to shorten the name.
  • you could either use BEM .crmCaseType__activitySet-label--edit or follow the convention in the file: .crmCaseType .edit-activitySet-label.

This comment has been minimized.

Copy link
@deb1990

deb1990 Apr 19, 2018

Author Contributor

Following existing convention, crmCaseType-edit-activity-set

border: 0 !important;
border-bottom: 1px solid #666 !important;
box-sizing: border-box;
float: left;

This comment has been minimized.

Copy link
@reneolivo

reneolivo Apr 19, 2018

Contributor

why is float: left important?

This comment has been minimized.

Copy link
@deb1990

deb1990 Apr 19, 2018

Author Contributor

It is not required. Removed. Apologies.

<a href="#acttab-{{$index}}">
<span ng-if="!activitySet.isInEditMode">{{ activitySet.label }}</span>
<input ng-if="activitySet.isInEditMode"
class="crmCaseType-edit-actset"

This comment has been minimized.

Copy link
@reneolivo

reneolivo Apr 19, 2018

Contributor

please refer to the class name comment.

This comment has been minimized.

Copy link
@deb1990

deb1990 Apr 19, 2018

Author Contributor

Please refer above comment.

<span class="crm-i fa-trash" title="{{ts('Remove')}}"
ng-hide="activitySet.name == 'standard_timeline'"
ng-click="removeItem(caseType.definition.activitySets, activitySet)">{{ts('Remove')}}</span>
ng-click="removeItem(caseType.definition.activitySets, activitySet)"></span>

This comment has been minimized.

Copy link
@reneolivo

reneolivo Apr 19, 2018

Contributor

Why did you remove this text? I know it's not visible anyways, but maybe it was there for accessibility reasons (not 100% sure).

This comment has been minimized.

Copy link
@deb1990

deb1990 Apr 19, 2018

Author Contributor

It was visible before, but I removed it to save space. It is also mentioned in the DOC
Removed the "Remove" label from the trash icon, because otherwise it was taking lot of space.

This comment has been minimized.

Copy link
@reneolivo

reneolivo Apr 19, 2018

Contributor

Ah right sorry 🤦‍♂

This comment has been minimized.

Copy link
@colemanw

colemanw Apr 20, 2018

Member

For accessibility, I suggest adding back a title "Remove Activity Set"

@reneolivo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

I have tested the changes locally, and they work as intended:

anim

@reneolivo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

@deb1990 deb1990 changed the title (WIP) Add inplace edit for timeline name Add inplace edit for timeline name Apr 19, 2018

@jamienovick

This comment has been minimized.

Copy link

commented Apr 20, 2018

@deb1990 please see core example of edit in place - we should use the same approach:

inplaceedit

This can be found on a clean CiviCRM on the activity tab of the contact record.

We should also make sure this change we are making is suitable for Shoreditch theme.

@colemanw

This comment has been minimized.

Copy link
Member

commented Apr 20, 2018

Since this is an Angular context, I think we should move this code into core and out of the Case extension, as it is generally applicable.
https://github.com/civicrm/org.civicrm.civicase/blob/master/ang/civicase/Utils.js#L402-L432

@reneolivo

This comment has been minimized.

Copy link
Contributor

commented May 2, 2018

@colemanw @jamienovick I was investigating about the crm-editable component that already exists in CiviCase and that is also available through CRM.$('...').crmEditable(), but that functionality is a bit different than what we have here.

crm-editable expects to edit a field from an existing entity (for example: the display name of a contact) and it will automatically save the field to the corresponding entity.

In this case, the activity set name is not part of an entity per se, but part of some definition data that is stored as XML for the case type.

The solution I'm providing in this commit: adea3e2 makes the activity set name to look a bit more like crm-editable:

anim

Please let me know if this is satisfactory.

@jamienovick

This comment has been minimized.

Copy link

commented May 2, 2018

I'm happy if Coleman is. Looks good great job.

@colemanw

This comment has been minimized.

Copy link
Member

commented May 8, 2018

I tried it out and the UX is slightly odd. I would expect that:

  • Clicking outside the box should cancel the edit (it doesn't)
  • Pressing ESC should cancel the edit (it doesn't)
  • The text inside the box should be selectable (clicking and dragging on the text does not select it, instead it pastes)

@deb1990 deb1990 force-pushed the compucorp:timeline-name-edit-inplace branch from e37530e to 9a99ed2 May 14, 2018

@colemanw

This comment has been minimized.

Copy link
Member

commented May 14, 2018

  • Pressing enter ought to save instead of creating a line break.
  • The cancel button is missing (see standard in-place edit UI).

@deb1990 deb1990 force-pushed the compucorp:timeline-name-edit-inplace branch from edab16b to ec3fc22 May 15, 2018

@reneolivo
Copy link
Contributor

left a comment

When you click twice on the title it exits the edit mode. Can this be fixed?

@deb1990 deb1990 force-pushed the compucorp:timeline-name-edit-inplace branch from ec3fc22 to 43740a0 May 15, 2018

@deb1990

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2018

When you click twice on the title it exits the edit mode. Can this be fixed?

@reneolivo this is fixed.

@reneolivo
Copy link
Contributor

left a comment

anim

All good now

@deb1990

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2018

Thanks @reneolivo
@colemanw Thanks for all the feedback. They have been implemented, please take a look.

text-align: center;
width: 30px;
z-index: 1;
}

This comment has been minimized.

Copy link
@colemanw

colemanw May 15, 2018

Member

I'm not sure why this is necessary. If you reuse the classes from standard crm-editable then these buttons should "just work" and when a theme like Shoreditch comes in to restyle everything it won't need to make a special extra bit of css just for these 2 buttons.

This comment has been minimized.

Copy link
@colemanw

colemanw May 15, 2018

Member

Resolved this.

This comment has been minimized.

Copy link
@deb1990

deb1990 May 16, 2018

Author Contributor

Thanks

@colemanw

This comment has been minimized.

Copy link
Member

commented May 15, 2018

@colemanw

This comment has been minimized.

Copy link
Member

commented May 15, 2018

@reneolivo in the future please do not add merge commits to a PR. If you need to pull in changes from master do git pull origin master --rebase instead of git pull and then git push -f. They mess up PR management.

To tidy things up, I've rebased & squashed this PR into a single commit.

@colemanw colemanw force-pushed the compucorp:timeline-name-edit-inplace branch from ffdd363 to d7a470d May 15, 2018

@colemanw

This comment has been minimized.

Copy link
Member

commented May 15, 2018

@civicrm-builder retest this please

1 similar comment
@colemanw

This comment has been minimized.

Copy link
Member

commented May 15, 2018

@civicrm-builder retest this please

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented May 15, 2018

Test fails look related PhantomJS 2.1.1 (Linux 0.0.0).crmCaseType crmEditableTabTitle when ESCAPE key is pressed while typing.shows the pen icon
PhantomJS 2.1.1 (Linux 0.0.0).crmCaseType crmEditableTabTitle when ESCAPE key is pressed while typing.hides the save and cancel button
PhantomJS 2.1.1 (Linux 0.0.0).crmCaseType crmEditableTabTitle when ESCAPE key is pressed while typing.makes the title non editable
PhantomJS 2.1.1 (Linux 0.0.0).crmCaseType crmEditableTabTitle when ENTER key is pressed while typing.shows the pen icon
PhantomJS 2.1.1 (Linux 0.0.0).crmCaseType crmEditableTabTitle when ENTER key is pressed while typing.hides the save and cancel button
PhantomJS 2.1.1 (Linux 0.0.0).crmCaseType crmEditableTabTitle when ENTER key is pressed while typing.makes the title non editable
PhantomJS 2.1.1 (Linux 0.0.0).crmCaseType crmEditableTabTitle when ENTER key is pressed while typing.updates the title in angular context

@deb1990

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

@colemanw @eileenmcnaughton Fixed the unit tests.

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

merging per label

@eileenmcnaughton eileenmcnaughton merged commit bd27f44 into civicrm:master May 16, 2018

1 check passed

default Build finished.
Details

deb1990 added a commit to compucorp/civicrm-core that referenced this pull request May 17, 2018

deb1990 added a commit to compucorp/civicrm-core that referenced this pull request May 18, 2018

deb1990 added a commit to compucorp/civicrm-core that referenced this pull request May 18, 2018

deb1990 added a commit to compucorp/civicrm-core that referenced this pull request May 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.