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

This should fix bug #1892 in toolshed #2142

Merged
merged 3 commits into from Apr 13, 2016

Conversation

Projects
None yet
6 participants
@nturaga
Copy link
Contributor

commented Apr 12, 2016

screen shot 2016-04-12 at 5 30 43 pm

@dannon

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

(working w/ @nturaga directly, this is getting swapped over to a class+style def to be more reusable)

@galaxybot galaxybot added the triage label Apr 12, 2016

@galaxybot galaxybot added this to the 16.07 milestone Apr 12, 2016

@martenson

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

xref #1892

nturaga added some commits Apr 13, 2016

@nturaga

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2016

@galaxybot test this

@martenson

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

@dannon dannon merged commit d7353d3 into galaxyproject:dev Apr 13, 2016

2 of 4 checks passed

toolshed test Build finished.
Details
api test Test started.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished.
Details
@martenson

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

thanks for fixing this @nturaga

@nturaga nturaga deleted the nturaga:fix_1892_toolshed_commit_linebreaks branch Apr 13, 2016

@remimarenco

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2016

Is it on purpose we don't use the Less nesting feature? http://lesscss.org/features/#features-overview-feature-nested-rules

Could we do that instead?

@borderGrey: solid #DDDDDD 1px; 

.grid {
    tbody {
        td {
            line-height: @line-height-base;
            border-top: @borderGrey;
            border-bottom: @borderGrey;
            padding: 5px;
        }

        td:empty {
            padding: 0;
        }
    }

    thead {
        tr {
            height: 2em;
        }

        th {
            line-height: @line-height-base;
            background: @table-heading-bg;
            color: contrast(@table-heading-bg);
            border-top: solid @table-border-color 1px;
            border-bottom: solid @table-border-color 1px;
            padding: 5px;
            text-align: left;
            white-space: nowrap;
        }
    }

    tfoot td {
        background-color: #F8F8F8;
        border-top: @borderGrey;
        border-bottom: @borderGrey;
        padding: 5px;
    }

    .current {
        background-color: #EEEEFF;
    }
}
@dannon

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

@remimarenco I'm not aware of any particular reasons we don't use that style other than, perhaps, general familiarity with cascading and non-nested css style when we first implemented this.

It might be worth looking at refactoring the .less at some point to see if the nested style works well and is more readable. On the face of it, it looks like it might be, to me, though I don't know how high I'd put it on the priority list ;)

@martenson

This comment has been minimized.

Copy link
Member

commented Apr 13, 2016

@remimarenco It is not on purpose. Most of the .less files are ready for some refactoring using more of the less features. Mainly separating files for each JS app and providing better namespacing. We do not enforce any style though.

@remimarenco

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2016

Thanks for the answers! Added in my personal backlog then :p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.