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

Let users select log content without line numbers #5736

Merged
merged 1 commit into from Nov 13, 2015

Conversation

spadgett
Copy link
Member

@spadgett spadgett commented Nov 5, 2015

Add a mixin for browser prefixed user-select: none styles. Switch to a table for log content so user-select works properly in Safari.

/cc @jwforres

@@ -157,7 +156,7 @@ angular.module('openshiftConsole')

// Append the line to the document fragment buffer.
var line = logLineTemplate.cloneNode(true);
line.childNodes[0].childNodes[0].appendChild(document.createTextNode(lastLineNumber));
line.childNodes[0].appendChild(document.createTextNode(lastLineNumber));
Copy link
Contributor

Choose a reason for hiding this comment

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

s/childNodes[0]/firstChild/?

.flex(@columns: 0 0 60px); // set width of number column
.unselectable();
border-right: 1px lighten(@log-bg-color, 10%) solid;
min-width: 60px;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we actually need this much space, or can we shrink it further? might want to set white-space: nowrap; as well if you do

Copy link
Contributor

Choose a reason for hiding this comment

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

ideally, a short log would reserve the minimal space for single or double-digit lines, and a long log would grow the left column to fit 4,5,6 digit numbers

Copy link
Member Author

Choose a reason for hiding this comment

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

Need that much for 5 digits. I set that as a min to avoid the log content jumping to the right when we reach 100 or 1000 lines, but maybe it's not such a problem.

@jwforres
Copy link
Member

jwforres commented Nov 5, 2015

should we be doing width: 100% on the line text column, otherwise what happens when your first log line isn't very long?

also you double checked the word break stuff?

@liggitt
Copy link
Contributor

liggitt commented Nov 5, 2015

width: 100% on the line text column, otherwise what happens when your first log line isn't very long?

probably

@spadgett
Copy link
Member Author

spadgett commented Nov 5, 2015

@jwforres It still fills the width with short lines.

@spadgett
Copy link
Member Author

spadgett commented Nov 5, 2015

I checked word break. It's the same as before.

@liggitt
Copy link
Contributor

liggitt commented Nov 5, 2015

@jwforres It still fills the width with short lines.

in all browsers?

@spadgett
Copy link
Member Author

spadgett commented Nov 5, 2015

Ah ha, the table width isn't 100%, but you can't tell because there are no borders and the parent sets the background black.

@jwforres
Copy link
Member

jwforres commented Nov 5, 2015

ahhhh, wouldnt the hover effect be wrong then

@spadgett
Copy link
Member Author

spadgett commented Nov 5, 2015

Yes, fixing...

@spadgett spadgett changed the title Let users to select log content without line numbers Let users select log content without line numbers Nov 9, 2015
@jwforres jwforres added component/web lgtm Indicates that a PR is ready to be merged. labels Nov 10, 2015
@jwforres
Copy link
Member

LGTM

@jwforres
Copy link
Member

[merge]

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 6221080

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7100/)

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3988/) (Image: devenv-rhel7_2695)

@spadgett
Copy link
Member Author

re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 6221080

openshift-bot pushed a commit that referenced this pull request Nov 13, 2015
@openshift-bot openshift-bot merged commit 42ad235 into openshift:master Nov 13, 2015
@spadgett spadgett deleted the user-select-none branch November 13, 2015 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/web lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants