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.

Sign up for GitHub

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

Adding view full log in alerts button #2862

Merged
merged 1 commit into from
Dec 20, 2017
Merged

Adding view full log in alerts button #2862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@ ManageIQ.angular.app.controller('emsCommonFormController', ['$http', '$scope', '
} else {
$scope.updateAuthStatus(true);
}
miqService.miqFlash(data.level, data.message);
miqService.miqFlash(data.level, data.message, data.options);
miqSparkleOff();
});
});
Expand Down
51 changes: 51 additions & 0 deletions app/assets/javascripts/miq_flash.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ function add_flash(msg, level, options) {
throw("add_flash: unknown level: " + level);
}

if (options.long_alert) {
Copy link
Member

Choose a reason for hiding this comment

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

you should not depend on options here,
we can deduce that we need the new feature if msg is longer then some value:

var SHORT_MSG_LENGTH = 120;
var shortMsg;

if (msg.length > SHORT_MSG_LENGTH) { ...
shortMsg = msg.substring(0, SHORT_MSG_LENGTH) + '...';

Copy link
Contributor Author

@nimrodshn nimrodshn Dec 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

thing is @himdel didn't want to hardcode the length thats why we added the options. (it's enough as is we hardcode it in the backend)

Copy link
Member

Choose a reason for hiding this comment

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

It is still hard coded to 200 in manageiq L94

or am I missing something ?

Copy link
Contributor Author

@nimrodshn nimrodshn Dec 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Thats what I said..

You're also randomly hardcoding a 200 character limit - this is wrong because this will affect any existing messages long enough. And it is also wrong because the reasonable size will depend on the user's resolution, and chosen language (this is why Twitter still limits Chinese to 140 chars).

Copy link
Member

Choose a reason for hiding this comment

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

Currently the text in the line is too long ... so something is wrong here :-)
I would expect a flash to look like that:

Short view (only one or two lines in small screens)

Lorem ipsum dolor sit amet, per ne erat vitae sententiae, eam tollit sententiae ut, 
dicant eripuit suavitate an cum. Ea al...  Show More

Long view (show full message)

Lorem ipsum dolor sit amet, per ne erat vitae sententiae, eam tollit sententiae ut, 
dicant eripuit suavitate an cum. Ea alii elitr graecis has. Scripta iracundia cum eu, 
adipisci evertitur ea est. Mei in partem adversarium. Facete adipiscing necessitat
ibus qui at, an accusamus repudiare scripserit mea, odio dissentiunt ea sed. 

Show Less

@nimrodshn @ohadlevy @himdel WDYT ?

Copy link
Contributor Author

@nimrodshn nimrodshn Dec 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Short view (only one or two lines in small screens)

@yaacov it still only show's one line (regardless of the screen) , how short do you want the msg to be? half a line?

Copy link
Member

Choose a reason for hiding this comment

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

@nimrodshn
If text-overflow: ellipsis; does not work here (see gif in description) then you can go back to trancate by some constant, IMHO it's ok for this length to be more then one line on smaller screens, and it's also ok for this short version to be more short for some locales ( e.g. Chinese ), see above example.
@himdel WDYT ?

Copy link
Member

Choose a reason for hiding this comment

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

@yaacov it still only show's one line (regardless of the screen) , how short do you want the msg to be? half a line?

see above example.
If you look carefully you can see I show two lines in that example.

Copy link
Member

Choose a reason for hiding this comment

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

@yaacov it still only show's one line (regardless of the screen)

This is problematic on small devices, you sometimes want to show more then one line:

Big screen, one line is enough for short message:

This is a problem that can be fixed easily by pressing this button

Small screen, you may want two or three lines for short message:

This is a problem that
can be fixed easily by
pressing this button

If you show only one line:

This is a problem that...

This is not what I as user want to see :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

@yaacov Is this really a problem if the user can still expand the message to full length? Would your issue be solved if this was 2 lines instead of one?

As for https://github.com/ManageIQ/manageiq-ui-classic/pull/2862/files#r156909937 .. I'm not quite understanding what the issue is (or missing a screenshot where it goes wrong).

cls.alert += " text-overflow-pf";
}

var iconSpan = $('<span class="' + cls.icon + '"></span>');

var textStrong = $('<strong></strong>');
Expand All @@ -35,6 +39,17 @@ function add_flash(msg, level, options) {
var alertDiv = $('<div class="' + cls.alert + '"><button class="close" data-dismiss="alert"><span class="pficon pficon-close"></span></button></div>');
alertDiv.append(iconSpan, textStrong);

// if options.long alert is given than add a `See More` button to the alert div to allow user the get more details of the error/alert.
if (options.long_alert) {
var detailsLinkTxt = __("View More");
var detailsLink = $('<div class="alert_expand_link"><strong><a href="#">' + detailsLinkTxt + '</a></strong></div>');
var params = {clicked: false, alert_elem: alertDiv, link: detailsLink};
detailsLink.on('click', function() {
expandAlert(params);
});
alertDiv.append(detailsLink);
}

var textDiv = $('<div class="flash_text_div"></div>');

textDiv.append(alertDiv);
Expand All @@ -46,12 +61,48 @@ function add_flash(msg, level, options) {
}

$('#flash_msg_div').append(textDiv).show();

// remove dangling 'Show More' link when the alert msg is short.
if ( options.long_alert && ! checkElipsis(alertDiv) ) {
detailsLink.hide();
}
}

function clearFlash() {
$('#flash_msg_div').empty();
}

function checkElipsis(element) {
var found = false;
var $c = element
.clone()
.css({display: 'inline-block', width: 'auto', visibility: 'hidden'})
.appendTo('body');
if ( $c.width() > element.width() ) {
found = true;
}

$c.remove();

return found;
}

function expandAlert(params) {
var viewMoreTxt = __("View More");
var viewLessTxt = __("View Less");
if (! params.clicked) {
params.clicked = true;
params.alert_elem.removeClass('text-overflow-pf');
params.alert_elem.addClass('text-vertical-overflow-pf');
params.link.find('a').text(viewLessTxt);
} else {
params.clicked = false;
params.alert_elem.removeClass('text-vertical-overflow-pf');
params.alert_elem.addClass('text-overflow-pf');
params.link.find('a').text(viewMoreTxt);
}
}

function _miqFlashLoad() {
return JSON.parse(sessionStorage.getItem('flash_msgs') || '[]');
}
Expand Down
4 changes: 2 additions & 2 deletions app/assets/javascripts/services/miq_service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ ManageIQ.angular.app.service('miqService', ['$timeout', '$document', '$q', 'API'
miqSparkleOff();
};

this.miqFlash = function(type, msg) {
this.miqFlash = function(type, msg, options) {
miqService.miqFlashClear();
add_flash(msg, type);
add_flash(msg, type, options);
};

// FIXME: usually we just hide it, merge the logic
Expand Down
5 changes: 5 additions & 0 deletions app/assets/stylesheets/patternfly_overrides.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -913,6 +913,11 @@ img.tiny { margin: 0; padding: 0;width:20px;height:20px; border: 0; vertical-ali
border: 2px dashed #ddd;
}

.text-vertical-overflow-pf {
overflow-y: auto;
max-height: 17ex;
}

/// ===================================================================
/// end misc styling
/// ===================================================================
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/mixins/ems_common_angular.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def render_validation_result(result, details)
level = :error
end

render_flash_json(msg, level)
render_flash_json(msg, level, :long_alert => true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nowsg is unlimited, perhaps ruby should still truncate to some large limit here, to avoid sending megabyte responses to UI.

Copy link
Contributor Author

@nimrodshn nimrodshn Dec 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@cben so truncate at 500 chars here?

Copy link
Contributor

@himdel himdel Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is this addressed by https://github.com/ManageIQ/manageiq/pull/16550/files#diff-f9cace1fe4fdcec66f962b23f2110fd6R337 now? (I think it is, making sure :).)

end

def create
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/mixins/generic_form_mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ def delete_action
end
end

def render_flash_json(msg, level = :success)
render :json => {:message => msg, :level => level}
def render_flash_json(msg, level = :success, options = {})
render :json => {:message => msg, :level => level, :options => options}
end
end
end
20 changes: 20 additions & 0 deletions spec/javascripts/miq_flash_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,24 @@ describe('miq_flash.js', function() {
expect($('#flash_msg_div').html()).toEqual('');
});
});
describe('longAlert', function() {
beforeEach(function() {
var html = '<html><head></head><body><div id="flash_msg_div"></div></body></html>';
setFixtures(html);
});

it('displays flash_msg_div "View More" button', function() {
// Expect alert msg div to add the "See More" button to the div.
add_flash("Lorem ipsum dolor sit amet, usu ei mollis vivendum, ancillae indoctum philosophia an pri, affert partiendo cum ne. Nec animal tincidunt philosophia ea. Ne mea liber gloriatur, ignota dictas mei ne. Omittam eleifend consequuntur vix eu, everti accusata accommodare et eam. Ut vidit semper instructior duo, usu in autem inermis. Viris pertinax constituto per id, at debet apeirian persecuti has. Nostrum expetenda qui ad, mazim iriure id duo, est alii wisi at.", 'error' , {long_alert: true});
var flash_msg_div = '<div class="flash_text_div"><div class="alert alert-danger text-overflow-pf"><button class="close" data-dismiss="alert"><span class="pficon pficon-close"></span></button><span class="pficon pficon-error-circle-o"></span><strong>Lorem ipsum dolor sit amet, usu ei mollis vivendum, ancillae indoctum philosophia an pri, affert partiendo cum ne. Nec animal tincidunt philosophia ea. Ne mea liber gloriatur, ignota dictas mei ne. Omittam eleifend consequuntur vix eu, everti accusata accommodare et eam. Ut vidit semper instructior duo, usu in autem inermis. Viris pertinax constituto per id, at debet apeirian persecuti has. Nostrum expetenda qui ad, mazim iriure id duo, est alii wisi at.</strong><div class="alert_expand_link" style="display: none;"><strong><a href="#">View More</a></strong></div></div></div>'
expect($('#flash_msg_div').html()).toEqual(flash_msg_div);
});

it('does not display flash_msg_div "See More" button', function() {
// Expect alert msg div to *not add* the "See More" button to the div.
add_flash("This is a really long alert!", 'error');
var flash_msg_div = '<div class="flash_text_div"><div class="alert alert-danger"><button class="close" data-dismiss="alert"><span class="pficon pficon-close"></span></button><span class="pficon pficon-error-circle-o"></span><strong>This is a really long alert!</strong></div></div>'
expect($('#flash_msg_div').html()).toEqual(flash_msg_div);
});
});
});