Code review comment for lp:~danilo/launchpad/bug-423836-a

У пет, 11. 09 2009. у 07:18 +0000, Michael Nelson пише:

> >> This CSS could possibly be much simpler if you put
> >> the 'translation-stats' class on the table element (instead of on the
> >> tbody). Then you could remove the overall-translation-stats class and do:
> >
> > That's actually what I started with. However, fighting legacy CSS code
> > is hell. 'table.listing tbody td' takes precedence over
> > 'table.translation-stats tbody td'. I am not yet ready to do away with
> > class='listing' on these tables, though, simply because I am not trying
> > to solve all the problems with these pages. It's mostly mechanical
> > changes, and no real redesign.
>
> OK. It's up to you, but I'm guessing:
>
> table.listing.translation-stats tbody td {...}

> would take precedence (ie. selecting both classes - as it's more
> specific). I'm not fussed either way - what you've got works, and it's
> not a huge improvement to remove one class.

Actually, it seems the problem was the 'tr' I had in when I tried this.
It "suddenly" just works.

Btw, I wouldn't think of combining classes like this, so thanks for the
pointer: I am sure it'll be useful in the future, but the simpler stuff
works here :)

> >>> +tbody.translation-stats td {
> >> table.translation-stats td {...
> >>
> >> (that way you don't need a separate rule for the tfoot below.)
> >
> > Actually, the 'cascading' of CSS seems to cause me problems in at least
> > Epiphany (xulrunner 1.9): if I set the rule up like that (that's what I
> > started with), it doesn't get picked up. I'll play a bit more with CSS
> > and will follow up.
> >
> > It's good to know that even CSS precedence rules differ between
> > browsers. :(
>
> Really? That is very strange. Try the more specific one I mentioned
> above - selecting both classes - I'd be interested to know why it was
> been over-ruled... is there something like firebug for epiphany?

Oh, it seems they don't differ: I was using a CSS rule with 'tr' in it
as well, and that probably caused problems. With that removed, it works
in Epiphany as well.

Also, I was under the impression that you tried this out in Firefox 3.5:
I did try this out in whatever-firefox-is-in-jaunty with firebug (which
is how I've seen that it doesn't really work properly). There is not
yet anything like firebug for epiphany, so in hard cases, I resort to
firefox :)

> Well *sorrrry* - I'd forgotten it was you helping barry with the rules -
> in previous reviews people haven't been aware. Next time I'll just keep
> my mouth shut ;)

Not at all! I think you should keep mentioning it: it's important that
everybody is aware of it.

> > Perhaps it'd make some sense for me to provide Navigation class for
> > ITranslationGroupSet, just so it's more consistent and so no confusion
> > arises in the future. Or perhaps just comment that this doesn't show up
> > in the request.traversed_objects because it doesn't provide Navigation
> > class.
> >
> > What do you think?
>
> I think a comment would be fine - it would have cleared up my confusion.

Done.

> No problem - I hope you didn't feel patronized by me pointing out things
> that were already obvious to you.

Don't worry at all: it's better to state the obvious than not and then
worry if somebody knows about something or not. I've long learned not
to be fussed about that with reviews (it was tough the first few months
in LP development :), because I realized that it helps me improve my
style as well.

For instance, you may have thought it obvious to combine classes like
you did above, but I didn't. So, keep up the good work!

Cheers,
Danilo