privly / privly-applications

The central repository for Privly Injectable Applications.

MIT License

19 stars 48 forks source link

Updated the cleartext element #337

Closed opg7371 closed 8 years ago

opg7371 commented 8 years ago

Updated the size of cleartext element in the top.css file Please review .....:)

317

smcgregor commented 8 years ago

Hi Piyush, thanks for tackling this!

Can you post a screen capture and if it looks good I'll give it a test on my laptop and phone?

opg7371 commented 8 years ago

sorry for the delay.. cleartext-content Too much big font will too look like a mess. Thus i kept it normal. Please tell me if i should increase the size....

However for a large font-size i have created a pull request.... please review it too........https://github.com/privly/privly-applications/pull/346

smcgregor commented 8 years ago

Please open a single pull request for each issue and iterate based on comments until we merge it. If you want to show an alternative font, just post a second picture and describe it.

smcgregor commented 8 years ago

I think the PR with the larger font might be better, but I would need to see it on a smaller screen (a phone). It might be good to conditionally set the font size based on the screen resolution (using @media rules or another method).

opg7371 commented 8 years ago

cleartext-min This is how it looks on phone with a larger font(23px). Yes i do agree with you regarding the font-size should change based on screen resolution

smcgregor commented 8 years ago

I think the larger font looks better.

opg7371 commented 8 years ago

This one consist of the larger font-size (23px) and looks like this in the browser. content-large

Please review...:)

smcgregor commented 8 years ago

Looks good. Can you squash your commits into a single commit and I will merge?

opg7371 commented 8 years ago

Sir i guess i did it wrong...sorry will be soon updating it now..... Ignore this squash of commits

opg7371 commented 8 years ago

A more Updated pull request here ....#354