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

(#1261)Style Fixes in Privacy Page #1260

Merged
merged 1 commit into from
Dec 8, 2018
Merged

(#1261)Style Fixes in Privacy Page #1260

merged 1 commit into from
Dec 8, 2018

Conversation

saarthakchats
Copy link
Member

@saarthakchats saarthakchats commented Dec 5, 2018

Fixes #1261

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream master branch.
  • I have added necessary documentation (if appropriate)
  • Added Surge preview link

Changes proposed in this pull request:

Before

screenshot 2018-12-05 at 2 29 47 pm

^^^ There is text behind that footer ^^^

screenshot 2018-12-05 at 2 31 17 pm

^^^ Unnecessarily looks like a link --> misleading ^^^ ## After

screenshot 2018-12-05 at 2 56 29 pm

@codecov
Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #1260 into development will decrease coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #1260      +/-   ##
===============================================
- Coverage        54.78%   54.64%   -0.15%     
===============================================
  Files               51       51              
  Lines             1400     1400              
  Branches           171      171              
===============================================
- Hits               767      765       -2     
- Misses             529      533       +4     
+ Partials           104      102       -2
Impacted Files Coverage Δ
src/app/services/intelligence.service.ts 75% <0%> (-8.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75f57a8...4216422. Read the comment docs.

Copy link
Member

@pythongiant pythongiant left a comment

Choose a reason for hiding this comment

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

Please provide a screenshot

@saarthakchats
Copy link
Member Author

Waiting for the preview myself 😅

@pythongiant
Copy link
Member

pythongiant commented Dec 5, 2018

open an issue too
use ng serve to check your changes
please dont test in production and add [WIP] to the title if you are still working on it

@saarthakchats saarthakchats changed the title Style Fixes in Privacy Page (#1261)Style Fixes in Privacy Page Dec 5, 2018
Copy link
Member

@praveenojha33 praveenojha33 left a comment

Choose a reason for hiding this comment

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

Do not use inline CSS.

@saarthakchats
Copy link
Member Author

@praveenojha33 Done

@praveenojha33
Copy link
Member

praveenojha33 commented Dec 6, 2018

@saarthakchats Please update your issue and make it relevant and detailed. As I can see you are fixing problem related to hidden text and wrong class name. Please mention same in issue.

@saarthakchats
Copy link
Member Author

Don3 @praveenojha33

@pythongiant
Copy link
Member

image
Still looks broken

@Pipe-Runner
Copy link
Contributor

screenshot-pr-1260-fossasia-susper surge sh-2018 12 07-22-37-48
screenshot-pr-1260-fossasia-susper surge sh-2018 12 07-22-37-40

It seems that the fix you did simply removed the underline, which is good to begin with, but I think you should change the stying of the text to make it look like a heading. Also, make the same changes in terms as well as changing styles only in a single place would make it look inconsistent.

Good luck @saarthakchats

@saarthakchats
Copy link
Member Author

all done @AakashMallik

@praveenojha33 praveenojha33 merged commit b8e5e5f into fossasia:development Dec 8, 2018
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.

Problems in the Privacy page
4 participants