Skip to content
This repository has been archived by the owner on Jul 6, 2020. It is now read-only.

Modified get involved page to the current version of EvalAI #136

Merged
merged 14 commits into from
Jul 4, 2019

Conversation

Sanji515
Copy link
Member

@Sanji515 Sanji515 commented Jun 2, 2019

Minor changes related to css

  • Changed the link color from red to yellow and some font related css to match with the theme of current version

  • Link to live demo: http://pr-136-evalai.surge.sh


Old UI Screenshot 1:

Screenshot from 2019-06-24 16-32-42

Modified UI Screenshot 1:

Screenshot from 2019-07-03 19-06-02

Modified UI Screenshot 2:

Screenshot from 2019-06-25 21-30-59

@codecov-io
Copy link

codecov-io commented Jun 2, 2019

Codecov Report

Merging #136 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #136      +/-   ##
=========================================
- Coverage   53.22%   53.2%   -0.02%     
=========================================
  Files          55      55              
  Lines        2463    2462       -1     
  Branches      261     261              
=========================================
- Hits         1311    1310       -1     
  Misses       1077    1077              
  Partials       75      75
Impacted Files Coverage Δ
.../components/get-involved/get-involved.component.ts 100% <100%> (ø) ⬆️
Impacted Files Coverage Δ
.../components/get-involved/get-involved.component.ts 100% <100%> (ø) ⬆️

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 75e8c6f...d125007. Read the comment docs.

Copy link
Member

@Shekharrajak Shekharrajak left a comment

Choose a reason for hiding this comment

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

I don't see much difference. Can you let us know what are the changes you did in current version of EvalAI? Make sure you write your changes in PR description properly.

@lunayach
Copy link
Member

Hi, @Sanji515 could you respond to @Shekharrajak and add some tests before we merge this?

@Sanji515
Copy link
Member Author

Make sure you write your changes in PR description properly

@Shekharrajak @lunayach Updated the PR description, please have a look

@Sanji515
Copy link
Member Author

add some tests before we merge this

done, @lunayach please review

@Sanji515
Copy link
Member Author

@lunayach @Shekharrajak please review

@lunayach
Copy link
Member

Very minor things, but better if we don't leave them for the last moment.

Screenshot from 2019-06-25 14-34-50

UL list having "Get Involved", "CloudCV Website" etc. seems misaligned. (IMO)

Screenshot from 2019-06-25 14-34-30

Should we have a border separating the paragraph above and "Report issues"? Also little too much whitespaces around the centred text. (IMO)

@lunayach
Copy link
Member

lunayach commented Jun 25, 2019

Merging #136 into master will decrease coverage by 0.01%.

@Sanji515 if merging the code does not increase the code-coverage, we should make sure that the coverage is not decreased at least.

@Sanji515
Copy link
Member Author

Sanji515 commented Jun 25, 2019

@lunayach I think that's the only test case can be written for this static page, please let me know me if I'm missing something

@lunayach
Copy link
Member

Okay, @Sanji515. It's fine for this PR, but please take a note of it for future PR's.

@Sanji515
Copy link
Member Author

@lunayach yeah I do check report but even after adding that one test the report hasn't updated, it's still the same.

@Sanji515
Copy link
Member Author

UL list having "Get Involved", "CloudCV Website" etc. seems misaligned

@lunayach hey I think footer is going to be change a/c to our current version, so there's no need to do that

Should we have a border separating the paragraph above and "Report issues"? Also little too much whitespaces around the centred text

done and updated the PR description, please review

@Sanji515
Copy link
Member Author

@lunayach please review

@Sanji515
Copy link
Member Author

Sanji515 commented Jul 3, 2019

@lunayach can you please take a look at this?

@lunayach
Copy link
Member

lunayach commented Jul 3, 2019

HI @Sanji515, by the border in the above comment, I meant horizontal line separating the section beginning with "Report issues" and the introductory text above it. I guess, you may have misunderstood and underlined all the headings like "Report issues", "Press" instead. Those may not be required and could be removed. (IMO)

@Sanji515
Copy link
Member Author

Sanji515 commented Jul 3, 2019

@lunayach sorry that I misunderstood the things, I've removed the border bottom of headings and added a border above the first heading and below the intro text

Please review

Copy link
Member

@lunayach lunayach left a comment

Choose a reason for hiding this comment

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

Looks good to me. LGTM.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants