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

moved repeated-prefix and repeated-time classes to tr #868

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Vaesper
Copy link
Contributor

@Vaesper Vaesper commented Nov 11, 2016

Also moved the highlight class from the prefix to the tr and modified the repeated-time expression to apply to a timestamp within 3 minutes of the previous one. If desired, this number can be tweaked.

CSS in the themes has been updated to match and this breaks the repeated prefix suppression in the wiki. Proposed update for that (with same flaws as current version):

.repeated-prefix td.prefix a span {
    position: relative;
    left: -1000px;
}

.repeated-prefix td.prefix a span.hidden-bracket,
.repeated-prefix.highlight td.prefix a span.hidden-bracket {
    position: absolute;
    left: -1000px;
}

.repeated-prefix td.prefix a span:nth-last-of-type(2):after {
    content:"↪";
    position: relative;
    left: 1000px;
}

.repeated-prefix.highlight td.prefix a span {
    position: initial;
    left: initial;
}

.repeated-prefix td.prefix a span.hidden-bracket:after,
.repeated-prefix.highlight td.prefix a span:after {
    content: initial;
    position: initial;
    left: initial;
}

Copy link
Member

@lorenzhs lorenzhs left a comment

Choose a reason for hiding this comment

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

Thanks, this looks nice, but the repeated-timestamp logic is hard to follow. It would probably be better to do (UNIX) timestamp diffing here, and have the timestamp exposed in the model, rather than parsing formatted timestrings once more

@@ -305,13 +305,13 @@ <h4 class="panel-title">
</tr>
</tbody>
<tbody ng-repeat="bufferline in bufferlines">
<tr class="bufferline">
<tr class="bufferline" ng-class="::{'repeated-prefix': bufferline.prefixtext==bufferlines[$index-1].prefixtext, 'repeated-time': (((60*+bufferline.shortTime.substr(0, bufferline.shortTime.indexOf(':')) + +bufferline.shortTime.substr(bufferline.shortTime.indexOf(':')+1)) - (60*+bufferlines[$index-1].shortTime.substr(0, bufferlines[$index-1].shortTime.indexOf(':')) + +bufferlines[$index-1].shortTime.substr(bufferlines[$index-1].shortTime.indexOf(':')+1)) + 1440) % 1440) < 3, 'highlight': bufferline.highlight}">
Copy link
Member

Choose a reason for hiding this comment

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

This should be an attribute of the bufferline. It's impossible to follow the logic when crammed onto a single line like this

<span class="cof-chat_time cob-chat_time coa-chat_time" ng-bind-html="::bufferline.formattedTime"></span>
</span>
</td>
<td class="prefix"><span ng-class="::{'repeated-prefix': bufferline.prefixtext==bufferlines[$index-1].prefixtext}"><a ng-click="addMention(bufferline.prefix)"><span class="hidden-bracket">&lt;</span><span ng-repeat="part in ::bufferline.prefix" ng-class="::part.classes" ng-bind="::part.text|prefixlimit:25"></span><span class="hidden-bracket">&gt;</span></a></span></td><!--
<td class="prefix"><span><a ng-click="addMention(bufferline.prefix)"><span class="hidden-bracket">&lt;</span><span ng-repeat="part in ::bufferline.prefix" ng-class="::part.classes" ng-bind="::part.text|prefixlimit:25"></span><span class="hidden-bracket">&gt;</span></a></span></td><!--
Copy link
Member

Choose a reason for hiding this comment

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

Does the outermost span have any function left? I guess we could just style the a it contains instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just removed it and it doesn't seem to break anything. Any styling of sub-elements of .prefix was already done to span-descendants so it applied to the span-children of the a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants