Skip to content

Issue #930 - Fix QR code style issues#936

Merged
miketaylr merged 2 commits intowebcompat:masterfrom
dshgna:issue-930
Mar 4, 2016
Merged

Issue #930 - Fix QR code style issues#936
miketaylr merged 2 commits intowebcompat:masterfrom
dshgna:issue-930

Conversation

@dshgna
Copy link
Copy Markdown
Contributor

@dshgna dshgna commented Mar 3, 2016

@magsout
Copy link
Copy Markdown
Member

magsout commented Mar 3, 2016

thanks @dshgna .

I will review tonight. But I'm no more a big fan for float right. Maybe a flex would be more appropriate here.

@miketaylr
Copy link
Copy Markdown
Member

I'll leave review to @magsout -- he's the resident CSS master.

I see there's a merge conflict, so might be good to rebase this branch against master. @dshgna let me know if you need help with that!

@dshgna
Copy link
Copy Markdown
Contributor Author

dshgna commented Mar 3, 2016

@miketaylr Thanks! Rebased the commit, but am unsure if the resolved merge conflicts give the intended results. Could you please verify from the screenshot?

#930 after conflict resolution

</span>
</div>
</div>
<div class="wc-Comment-content">

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@magsout
Copy link
Copy Markdown
Member

magsout commented Mar 3, 2016

@dshgna there is some trouble in small screen

  1. due to this issue Width of Comment on Small screen #940, not you fault (mine)

and

capture d ecran 2016-03-03 a 21 49 35

For fix the second issue. if you can add one <div></div> around all element except wc-Comment-qr.

And add justify-content: space-between in wc-Comment-header

capture d ecran 2016-03-03 a 22 01 07

except this minore issue, looks good.
good job @dshgna and thanks

@dshgna
Copy link
Copy Markdown
Contributor Author

dshgna commented Mar 4, 2016

Thanks @magsout for the great review! I will how to the page works on different device sizes before committing in future.

@magsout
Copy link
Copy Markdown
Member

magsout commented Mar 4, 2016

Awesome, LGTM @dshgna .

Just need to figure why travis have failed the tests

@magsout
Copy link
Copy Markdown
Member

magsout commented Mar 4, 2016

For me, it's ok to merged, there is no reason at all Travis failed. But I prefere to wait your validation @miketaylr

@miketaylr
Copy link
Copy Markdown
Member

Great, thanks!

miketaylr pushed a commit that referenced this pull request Mar 4, 2016
Issue #930 - Fix QR code style issues
@miketaylr miketaylr merged commit fac6a52 into webcompat:master Mar 4, 2016
@miketaylr
Copy link
Copy Markdown
Member

Just need to figure why travis have failed the tests

Travis will fail all PRs from a fork -- because we rely on having secret ENV VARs with some login and password info for the bot.

See https://docs.travis-ci.com/user/encryption-keys/ for how we encrypt the username and password, before storing them with Travis.

BUT I just saw https://docs.travis-ci.com/user/pull-requests#Security-Restrictions-when-testing-Pull-Requests, which shows how we could run a subset of tests that don't require auth, which might be a nice compromise!

@dshgna dshgna deleted the issue-930 branch March 15, 2016 04:55
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.

3 participants