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

Issue 387 remplacing links by icons #546

Merged

Conversation

Malmousque
Copy link
Contributor

@Malmousque Malmousque commented Mar 6, 2019

This PR replaces links, particulary in tabs, by icons into the 5 modules, some icons was already in use in some place, the idea is to improve and make the UX coherent.

coralicons3

5 news CSS classes have been added : .removeIcon .editIcon .updateIcon .addIcon .addIconTab.
All the icons was already in use. Most of them was only in the Resources modules images folder, I add them in the others images folders.

To update
coraladd

To add
coraladding

To remove
coralremove

To edit
coraledit

Is the update and edit function have a real separate function ?

jeffnm and others added 9 commits February 14, 2019 09:19
…ere is already icons for modifying/delete. The most of changes are in ressources/ajax_htmldata folder, 3 news css class (style.css) : div.ImportElement, div.addElement, div.adminHeader, with flexbox
…licensing, usage modules. Most of the changes in the ajax_HTMLdata and ajax_forms and admin file. News css classes : editIcon, AddIcon...
…. The ajax_htmldata files of the licensing, organizations, resources and usage modules have been amended, the css classes addIcon,adminAddIcon, removeIcone, AdminRemoveIcon, editIcon, adminEditIcon, adminUploadIcon and alignIcon have been created. alignIcon uses flexbox. Some links remain for a better understanding of users.
…. The ajax_htmldata files of the licensing, organizations, resources and usage modules have been amended, the css classes addIcon,adminAddIcon, removeIcone, AdminRemoveIcon, editIcon, adminEditIcon, adminUploadIcon and alignIcon have been created. alignIcon uses flexbox. Some links remain for a better understanding of users.
…. The ajax_htmldata files of the licensing, organizations, resources and usage modules have been amended, the css classes addIcon,adminAddIcon, removeIcone, AdminRemoveIcon, editIcon, adminEditIcon, adminUploadIcon and alignIcon have been created. alignIcon uses flexbox. Some links remain for a better understanding of users. Update(the 05 march 19) : add icons in the management module
@veggiematts veggiematts added ux Related to User Experience enhancement This is an enhancement (not a bug) labels Mar 6, 2019
@@ -203,7 +203,7 @@
echo "<td><div id='attachment_short_" . $attachment->attachmentID . "'>" . substr($attachmentText, 0,200);

if (strlen($attachmentText) > 200){
echo "...&nbsp;&nbsp;<a href='javascript:showFullAttachmentText(\"" . $attachment->attachmentID . "\");'>"._("more...")."</a>";
echo "...&nbsp;&nbsp;<a href='javascript:showFullAtt break;achmentText(\"" . $attachment->attachmentID . "\");'>"._("more...")."</a>";
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo ?

@@ -354,7 +354,7 @@ function submitQualifier(){


function showAdd(tableName){
$('#span_new' + tableName).html("<input type='text' name='new" + tableName + "' id='new" + tableName + "' class='adminAddInput' /> <a href='javascript:addData(\"" + tableName + "\");'>"+_("add")+"</a>");
$('#span_new' + tableName).html("<input type='text' name='new" + tableName + "' id='new" + tableName + "' class='adminAddInput' /> <a href='javascript:addData(\"" + tableName + "\");'><img id='addCurrency' src='images/plus.gif' title='add' /></a>");
Copy link
Contributor

Choose a reason for hiding this comment

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

Translation

<a href='ajax_forms.php?action=getNoteForm&height=233&width=410&tab=Access&entityID=<?php echo $resourceAcquisitionID; ?>&resourceNoteID=&modal=true' class='thickbox'><?php echo _("add new note");?></a>
<a href='ajax_forms.php?action=getNoteForm&height=233&width=410&tab=Access&entityID=<?php echo $resourceAcquisitionID; ?>&resourceNoteID=&modal=true' class='thickbox'><?php echo "<div class= 'addIconTab'><img id='Add' class='addIcon' src='images/plus.gif' title= '"._("Add")."' /></div>";?></a>
<?php }
if ($user->canEdit()){?>
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems misplaced.

?>
<div class="adminHeader">
<div></div>
<div class="addElement" style= "padding-bottom: 8px;"><?php echo "<a href='ajax_forms.php?action=getAdminAlertDaysForm&alertDaysInAdvanceID=&height=128&width=260&modal=true' class='thickbox'><img id='addAlertDay' src='images/plus.gif' title='add day'/></a>";?></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Translation

<div class="adminRightHeader"><?php echo _("Users");?></div>
<div class="addElement" style="margin-right: 4px"><a href='ajax_forms.php?action=getAdminUserUpdateForm&loginID=&height=275&width=315&modal=true' class='thickbox' id='addUser'><img id='addUserGroup' src='images/plus.gif' title='add User' /></a></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Translation

<?php if ($user->canEdit()){ ?>
<a href='ajax_forms.php?action=getNoteForm&height=233&width=410&tab=Product&entityID=<?php echo $resourceID; ?>&resourceNoteID=&modal=true' class='thickbox'><?php echo _("add new note");?></a>
<a href='ajax_forms.php?action=getNoteForm&height=233&width=410&tab=Product&entityID=<?php echo $resourceID; ?>&resourceNoteID=&modal=true' class='thickbox'><?php echo "<div class='addIconTab' ><img id='Add' src='images/plus.gif' title= '"._("Add")."' /></div>";?></a></div>
<?php if ($user->canEdit()){ ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems misplaced

…l issues : translation in licensing/js/admin and misplacing in resources/ajax_htmldata/getProductsDetails.php
@LibClaire
Copy link

Hi,
I've just had a first look at this. Patch applies without a problem and the icons are working as links.

I have a couple of comments:

  • Resources module: it would be nice if the "add" icon appeared next to the headings as it does in the other modules. I think it reads better that way and it is clearer what the icon is for.

  • Resources module: Under admin - alert settings there are two "add" icons, one for "add email" and one for "add day", it might be a good idea to have some text here to help identify them. (I have seen the title hover text, which is good, but I don't think it is enough when there is more than one option)

  • Licensing module: we have two different options, update and edit. My personal opinion is that we don't need to have two different icons/definitions here as they do the same thing? I came across this recently while translating and I think "edit" would be enough for all cases. Happy to have further feedback on this, perhaps there was a use case for it.

Otherwise I think this is good! I like the icons. :)

…text identify add icon for email and add icon for day under admin -alert setting ressources module
@Malmousque
Copy link
Contributor Author

Hi,
Sorry for not answering faster. The "add" icon now appears next to the headings in the ressource module, and I have added text to identify "add" icon in alert settings.
It would be maybe better to keep the question of "update/edit" for a next issue.
Thank you for your recommendations. :)

<div></div>
<div class="addElement" style= "padding-bottom: 8px;"><?php echo "<a href='ajax_forms.php?action=getAdminAlertDaysForm&alertDaysInAdvanceID=&height=128&width=260&modal=true' class='thickbox'><img id='addAlertDay' src='images/plus.gif' title='"._("add day")."'/></a>";?></div>
</div>
<div class="addElement" style= "padding-bottom: 8px;"><?php echo "<div><span class= 'addIconAlert'>Add a day: &nbsp;</span><a href='ajax_forms.php?action=getAdminAlertDaysForm&alertDaysInAdvanceID=&height=128&width=260&modal=true' class='thickbox'><img id='addAlertDay' src='images/plus.gif' title='"._("add day")."'/></a></div>";?></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Translation

<div class="addElement"><?php echo "<a href='ajax_forms.php?action=getAdminAlertEmailForm&alertEmailAddressID=&height=128&width=260&modal=true' class='thickbox'><img id='addAlertEmail' src='images/plus.gif' title='"._("add Email")."' /></a>";?></div>
<div class="adminHeaderAlert">
<?php echo "<div class='adminRightHeader'>"._("Alert Settings")."</div><br>";?>
<div class="addElement"><?php echo "<div><span class= 'addIconAlert'>Add an email: &nbsp;</span><a href='ajax_forms.php?action=getAdminAlertEmailForm&alertEmailAddressID=&height=128&width=260&modal=true' class='thickbox'><img id='addAlertEmail' src='images/plus.gif' title='"._("add Email")."' /></a></div>";?></div></br>
Copy link
Contributor

Choose a reason for hiding this comment

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

Translation

@Malmousque
Copy link
Contributor Author

"Add an email" and "Add a day" in the alert settings / ressources module can, now, be translated.

@LibClaire
Copy link

Hi Malmousque,
the changes look great!
I only have one other comment/question
Funds: here we have the 'down arrow' for an import. I am wondering if it is immediately clear what the icon means? To me an 'up arrow' would make more sense for an import and a 'down arrow' for download, but I know we have that in the other modules as 'update'... maybe we can address it in a new pull request but I wanted to flag it as possibly confusing.

@Malmousque
Copy link
Contributor Author

Hi LibClaire,

I'm agree with you. For now, I have done with the icons provided with coral, but we could use others. I will talk about that with matts.

@veggiematts
Copy link
Contributor

I think we can safely merge 'update' and 'edit' buttons to only have 'edit'.
I think having both wasn't made on purpose, but comes from development. We have two types of tables for admin preferences: standard and custom ones. All the standard ones use 'edit', and some of the custom ones use 'update'. We should make this consistent and only keep 'edit'.

@PaulPoulain
Copy link
Contributor

++ for merging update & edit and thus be consistent !

@LibClaire
Copy link

I would agree to just having 'edit'. I think it will be cleaner that way

@LibClaire
Copy link

Hi,
everything is looking good. The import icon is better!
One last comment. When going to edit something in a module that isn't resources, the heading of the pop up is still 'update'. Do we want to change the wording so that it is consistent?

@Malmousque
Copy link
Contributor Author

Hi LibClaire,

Yes, of course, I had to miss some "update".
Thanks

@Malmousque
Copy link
Contributor Author

Hi,
Sorry Claire, but I can't find the pop up you're talking about. All the 'Edit' icons seem to have an 'Edit' pop up. Could you tell me where it is ?

@veggiematts
Copy link
Contributor

Editing users in licensing for instance:
Capture d’écran_2019-03-29_13-52-42
It should be "Edit user"

@Malmousque
Copy link
Contributor Author

I modified the titles of the windows and harmonized the titles of the buttons (because for some, the title of the button and the window was in the same variable).
Have a nice WE !

@LibClaire
Copy link

Hi Malmousque,
I think it looks good now. :)

@veggiematts
Copy link
Contributor

Merging this to devel. Thank you @LibClaire and @Malmousque for the good work!

@veggiematts veggiematts merged commit 4a2bfb4 into coral-erm:development Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement (not a bug) ux Related to User Experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants