Skip to content
This repository has been archived by the owner on Jun 18, 2024. It is now read-only.

Commit

Permalink
Address review comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
@itsjeyd
itsjeyd committed Dec 1, 2015
1 parent 9e6e001 commit dcf0840
Show file tree
Hide file tree
Showing 7 changed files with 61 additions and 35 deletions.
4 changes: 4 additions & 0 deletions tests/integration/test_vectordraw.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ def check_vector_properties(
# Slope
vector_slope = vector_properties.find_element_by_css_selector(".vector-prop-slope")
self.assertFalse(vector_slope.is_displayed())
# "Update" button
update_button = vector_properties.find_element_by_css_selector('button.update')
update_button_disabled = update_button.get_attribute('disabled')
self.assertEquals(bool(update_button_disabled), input_fields_disabled)
else:
self.assert_not_present(
menu,
Expand Down
10 changes: 10 additions & 0 deletions vectordraw/public/css/vectordraw.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,16 @@
color: #ff0000;
}

.vectordraw_block .menu .vector-properties .disabled {

This comment has been minimized.

Sign in to view

Copy link
@bradenmacdonald

bradenmacdonald Dec 1, 2015

Member

No need to change for now, but did you try using the :disabled selector instead of a disabled class?
border: 1px solid #707070;
background-color: #ececec;
color: #868686;
}

.vectordraw_block .menu .vector-properties .disabled:hover {
background: none;
}

.vectordraw_block .action button {
height: 40px;
margin-right: 10px;
Expand Down
10 changes: 10 additions & 0 deletions vectordraw/public/css/vectordraw_edit.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,16 @@
float: right;
}

.vectordraw_edit_block .menu .controls .disabled {
border: 1px solid #707070;
background-color: #ececec;
color: #868686;
}

.vectordraw_edit_block .menu .controls .disabled:hover {

This comment has been minimized.

Sign in to view

Copy link
@bradenmacdonald

bradenmacdonald Dec 1, 2015

Member

For a disabled element, the :hover appearance should not change from the normal disabled appearance. If the color changes when you hover over it, that's usually a hint that the element is interactive.
background: none;
}

.vectordraw_edit_block .menu .vector-properties .vector-prop-list .row .vector-prop-name,
.vectordraw_edit_block .menu .vector-properties .vector-prop-list .row .vector-prop-tail,
.vectordraw_edit_block .menu .vector-properties .vector-prop-list .row .vector-prop-angle {
Expand Down
8 changes: 5 additions & 3 deletions vectordraw/public/js/vectordraw.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,17 +369,19 @@ function VectorDrawXBlock(runtime, element, init_args) {
}
// Enable input fields
$('.vector-properties input').prop('disabled', false);
// Enable buttons
$('.vector-properties button').removeClass('disabled').prop('disabled', false);
// Hide error message
$('.vector-prop-update .update-error', element).hide();
};

VectorDraw.prototype.resetVectorProperties = function(vector) {
// Clear current selection
this.element.find('.menu .element-list-edit option').attr('selected', false);
// Select default value
// Reset dropdown for selecting vector to default value
$('.menu .element-list-edit option[value="-"]', element).attr('selected', true);
// Reset input fields to default values and disable them
$('.menu .vector-prop-list input', element).prop('disabled', true).val('');
// Disable "Update" button
$('.vector-properties button').addClass('disabled').prop('disabled', true);
};

VectorDraw.prototype.isVectorTailDraggable = function(vector) {
Expand Down
56 changes: 28 additions & 28 deletions vectordraw/public/js/vectordraw_edit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ function VectorDrawXBlockEdit(runtime, element, init_args) {
this.resultMode = false;
this.settings = settings;
this.numberOfVectors = this.settings.vectors.length;
this.editableProperties = ['name', 'label', 'tail', 'length', 'angle'];
this.checks = [
'tail', 'tail_x', 'tail_y', 'tip', 'tip_x', 'tip_y', 'coords', 'length', 'angle'
];
Expand Down Expand Up @@ -212,45 +213,45 @@ function VectorDrawXBlockEdit(runtime, element, init_args) {
};

VectorDraw.prototype.addEditMenuOption = function(vectorName, idx) {
// 1. Find dropdown for selecting vector to edit
// Find dropdown for selecting vector to edit
var editMenu = this.element.find('.menu .element-list-edit');
// 2. Remove current selection(s)
// Remove current selection(s)
editMenu.find('option').attr('selected', false);
// 3. Create option for newly added vector
// Create option for newly added vector
var newOption = $('<option>')
.attr('value', 'vector-' + idx)
.attr('data-vector-name', vectorName)
.text(vectorName);
// 4. Append option to dropdown
// Append option to dropdown
editMenu.append(newOption);
// 5. Select newly added option
// Select newly added option
newOption.attr('selected', true);
};

VectorDraw.prototype.onRemoveVector = function(evt) {
if (!this.wasUsed) {
this.wasUsed = true;
}
// 1. Remove selected vector from board
// Remove selected vector from board
var vectorName = $('.element-list-edit', element).find('option:selected').data('vector-name');
var boardObject = this.board.elementsByName[vectorName];
this.board.removeAncestors(boardObject);
// 2. Mark vector as "deleted" so it will be removed from "vectors" field on save
// Mark vector as "deleted" so it will be removed from "vectors" field on save
var vectorSettings = this.getVectorSettingsByName(String(vectorName));
vectorSettings.deleted = true;
// 3. Remove entry that corresponds to selected vector from menu for selecting vector to edit
// Remove entry that corresponds to selected vector from menu for selecting vector to edit
var idx = _.indexOf(this.settings.vectors, vectorSettings),
editOption = this.getEditMenuOption("vector", idx);
editOption.remove();
// 4. Discard information about expected position (if any)
// Discard information about expected position (if any)
delete this.settings.expected_result_positions[vectorName];
// 5. Discard information about expected result (if any)
// Discard information about expected result (if any)
delete this.settings.expected_result[vectorName];
// 6. Reset input fields for vector properties to default values
// Reset input fields for vector properties to default values
this.resetVectorProperties();
// 7. Reset selected vector
// Reset selected vector
this.selectedVector = null;
// 8. Hide message about pending changes
// Hide message about pending changes
$('.vector-prop-update .update-pending', element).hide();
};

Expand All @@ -265,7 +266,6 @@ function VectorDrawXBlockEdit(runtime, element, init_args) {
// Update vector positions using positions from expected result
var expectedResultPositions = this.settings.expected_result_positions;
if (!_.isEmpty(expectedResultPositions)) {
// Loop over vectors and update their positions based on expected result positions
_.each(this.settings.vectors, function(vec) {
var vectorName = vec.name,
resultPosition = expectedResultPositions[vectorName];
Expand All @@ -279,27 +279,26 @@ function VectorDrawXBlockEdit(runtime, element, init_args) {
}, this);
this.board.update();
}
// Hide or disable operations that are specific to defining initial state
$(evt.currentTarget).prop('disabled', true);
// Hide or disable buttons for operations that are specific to defining initial state
$(evt.currentTarget).addClass('disabled').prop('disabled', true);
$('.add-vector', element).css('visibility', 'hidden');
$('.vector-prop-name input', element).prop('disabled', true);
$('.vector-prop-label input', element).prop('disabled', true);
$('.vector-remove button').hide();
// Reset vector properties to ensure a clean slate
this.resetVectorProperties();
// Show controls for opting in and out of checks
$('.checks', element).show();
// Disable input fields that users should not be able to interact with unless a vector is selected
_.each(['tail', 'length', 'angle'], function(propName) {
$('.vector-prop-' + propName + ' input', element).prop('disabled', true);
});
};

VectorDraw.prototype.resetVectorProperties = function(vector) {
// Select default value
$('.menu .element-list-edit option[value="-"]', element).attr('selected', true);
// Reset input fields to default values
$('.menu .vector-prop-list input', element).val('');
// Reset dropdown for selecting vector to default value
$('.element-list-edit option[value="-"]', element).attr('selected', true);
// Reset input fields and disable them
// (users should not be able to interact with them unless a vector is selected)
_.each(this.editableProperties, function(propName) {
$('.vector-prop-' + propName + ' input', element).prop('disabled', true).val('');
});
// Disable buttons
$('.vector-properties button').addClass('disabled').prop('disabled', true);
};

VectorDraw.prototype.getMouseCoords = function(evt) {
Expand Down Expand Up @@ -372,6 +371,8 @@ function VectorDrawXBlockEdit(runtime, element, init_args) {
}
// Enable input fields
$('.vector-properties input').prop('disabled', false);
// Enable buttons
$('.vector-properties button').removeClass('disabled').prop('disabled', false);
};

VectorDraw.prototype.updateChecks = function(vector) {
Expand Down Expand Up @@ -565,10 +566,9 @@ function VectorDrawXBlockEdit(runtime, element, init_args) {
$('.vector-prop-update .update-pending', element).hide();
// Get name of vector that is currently "selected"
var vectorName = String($('.element-list-edit', element).find('option:selected').data('vector-name')),
editableProperties = ['name', 'label', 'tail', 'length', 'angle'],
newValues = {};
// Get values from input fields
_.each(editableProperties, function(prop) {
_.each(this.editableProperties, function(prop) {
newValues[prop] = $.trim($('.vector-prop-' + prop + ' input', element).val());
});
// Process values
Expand Down
2 changes: 1 addition & 1 deletion vectordraw/templates/html/vectordraw.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ <h3>{{ self.vector_properties_label }}</h3>
</div>
<div class="row">
<div class="vector-prop vector-prop-update">
<button class="update">
<button class="update disabled" disabled="disabled">
<span class="update-label" aria-hidden="true">{% trans "Update" %}</span>
<span class="sr">{% trans "Update properties of selected element" %}</span>
</button>
Expand Down
6 changes: 3 additions & 3 deletions vectordraw/templates/html/vectordraw_edit.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,17 +200,17 @@ <h3>{{ self.vector_properties_label }}</h3>
</div>
<div class="row">
<div class="vector-prop vector-prop-update">
<button class="update">
<button class="update disabled" disabled="disabled">
<span class="update-label" aria-hidden="true">{% trans "Update" %}</span>
<span class="sr">{% trans "Update properties of selected element" %}</span>
</button>
<span class="update-pending">
{% trans "Pending changes." %}
{% trans "Unsaved changes." %}
</span>
<span class="update-error">{% trans "Invalid input." %}</span>
</div>
<div class="vector-prop vector-remove">
<button class="remove">
<button class="remove disabled" disabled="disabled">
<span class="remove-label" aria-hidden="true">{% trans "Remove" %}</span>
<span class="sr">{% trans "Remove selected element" %}</span>
</button>
Expand Down

2 comments on commit dcf0840

@bradenmacdonald
Copy link
Member

Choose a reason for hiding this comment

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

Looks good @itsjeyd! Please remove the :hover effect from the disabled buttons, and then this part of the work is 👍 from me - no need for further review.

@itsjeyd
Copy link
Member Author

@itsjeyd itsjeyd commented on dcf0840 Dec 2, 2015

Choose a reason for hiding this comment

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

@bradenmacdonald Thanks, :hover effect removed.

Please sign in to comment.