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

Make Debsources Responsive #62

Closed
wants to merge 33 commits into from

Conversation

devoxel
Copy link
Contributor

@devoxel devoxel commented Jun 6, 2016

Note that this is not ready to actually be pulled yet, but opening the PR allows people to easily keep an eye on changes without having to dig through stuff or host it on their local machine.

http://sourcesdev.debian.net should usually have an up to date hosted version of this, although recent commits may take time to pull.

@devoxel devoxel force-pushed the feature-responsive-templates branch 2 times, most recently from 375d2df to 94ca5f1 Compare June 7, 2016 14:22
@devoxel devoxel force-pushed the feature-responsive-templates branch from 94ca5f1 to d1e7ce9 Compare June 7, 2016 14:24
@devoxel devoxel force-pushed the feature-responsive-templates branch from 0519e76 to db36820 Compare June 16, 2016 17:27
Aaron Delaney added 3 commits June 17, 2016 20:35
This will help us get the code sorted. You can see my progress and see how I'm
coming along, right in git.
@devoxel devoxel force-pushed the feature-responsive-templates branch from c79e534 to e1d7f04 Compare June 19, 2016 14:53
@devoxel devoxel force-pushed the feature-responsive-templates branch from 6c58cc4 to d15ea7f Compare June 19, 2016 19:17
@@ -4,7 +4,8 @@
https://anonscm.debian.org/gitweb/?p=qa/debsources.git;a=blob;f=AUTHORS;hb=HEAD
License: GNU Affero General Public License, version 3 or above.
#}
{# copied from templates/index.html #}
{# XXX copied from templates/index.html #}
{# this should be a macro: repetitive code which makes change awkward #}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove unnecessary comments from the code :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this, will push up soon to the related branches. I'll review diffs and make sure there's not much more

@oorestisime
Copy link
Contributor

Heya,

i reviewed a bit your PR. Generally good work nice.
Besides the comments above here are some general notes:

  • some XXX fix this ... leftovers in the templates. please move them on to trello or delete them from there.. we should keep the templates clean
  • i guess you use a tool that autoformats or so? there are many changes that involve just adding a blank line somewhere or making one liner html into many lines. maybe its better to avoid this
  • i would leave debian.css unmodified. If you need to overwrite styles then you should probably do it in base.css like you did for boostrap. that way if a new version of debian.css comes out it will be trivial to update

Haven't taken the time yet to test this locally but i pulled on sourcesdev. The tests locally are ok for you?
In any case keep up the good work :)

@devoxel
Copy link
Contributor Author

devoxel commented Jun 25, 2016

Thanks for review. I already wrote a reply to this but lost it somehow... Anyways:

some XXX fix this ... leftovers in the templates. please move them on to trello or delete them from there.. we should keep the templates clean

I agree. Will do.

i guess you use a tool that autoformats or so? there are many changes that involve just adding a blank line somewhere or making one liner html into many lines. maybe its better to avoid this

I don't have an autoformatting tool, I changed some things I felt were indented badly. However, if you disagree with specific instances I will certainly be open to change.

i would leave debian.css unmodified. If you need to overwrite styles then you should probably do it in base.css like you did for boostrap. that way if a new version of debian.css comes out it will be trivial to update

If I recall correctly there was already some debsources specific stuff in there. However, you're right really, it's the better standard thing to do. I'll change over to that.

The only tests failing are failing on travis only and in master, as mentioned on the IRC. Nothing else is failing for me.

@devoxel devoxel closed this Jun 26, 2016
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