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

Home page company section #438

Closed

Conversation

muzammalrahim
Copy link
Contributor

Proposed changes

Checklist

  • Lint and unit tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added documentation to describe my feature.
  • I have squashed my commits into logic units.
  • [x ] I have described the changes in the commit messages.

Other information

@codecov
Copy link

codecov bot commented Jan 15, 2021

Codecov Report

Merging #438 (dab80ac) into master (7fcfe2f) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #438      +/-   ##
==========================================
- Coverage   83.31%   83.31%   -0.01%     
==========================================
  Files          27       26       -1     
  Lines        2482     2475       -7     
  Branches      253      250       -3     
==========================================
- Hits         2068     2062       -6     
  Misses        321      321              
+ Partials       93       92       -1     

File formatting fix
@muzammalrahim muzammalrahim marked this pull request as ready for review January 15, 2021 13:13
<a href="https://hosted.weblate.org/projects/collabora-online/"><img src="{% static "img/logo-collabora.svg" %}" alt="Collabora" /></a>
<div class="clear"></div>
<a href="https://hosted.weblate.org/projects/" class="button border small rev inline" target="_blank">{% trans "See all projects" %}</a>
<a href="{% url 'donate' %}#thanks" class="button border small rev inline">{% trans "See all supporters" %}</a>
<a href="https://hosted.weblate.org/projects/" class="button border small rev inline" style="text-decoration: none;" target="_blank">{% trans "See all projects" %}</a>
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid inline CSS (this one should not be necessary, the styling should be done by the classes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the inline CSS has been removed.

Copy link
Member

Choose a reason for hiding this comment

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

I can still see it in this place (style="text-decoration: none;").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I apologize for this, i have gotten rid of that now.

weblate_web/templates/snippets/reviews.html Outdated Show resolved Hide resolved
weblate_web/templates/snippets/reviews.html Outdated Show resolved Hide resolved
weblate_web/templates/snippets/reviews.html Outdated Show resolved Hide resolved
weblate_web/templates/snippets/reviews.html Outdated Show resolved Hide resolved
weblate_web/templates/snippets/reviews.html Outdated Show resolved Hide resolved
2) Testimonial has been changed to a single class.
3) Inline/excessive css has been removed.
4) Created and RTL css variant
@muzammalrahim
Copy link
Contributor Author

I have pushed two more commits for the mentioned fixes

weblate_web/static/style-rtl.css Outdated Show resolved Hide resolved
<a href="https://hosted.weblate.org/projects/collabora-online/"><img src="{% static "img/logo-collabora.svg" %}" alt="Collabora" /></a>
<div class="clear"></div>
<a href="https://hosted.weblate.org/projects/" class="button border small rev inline" target="_blank">{% trans "See all projects" %}</a>
<a href="{% url 'donate' %}#thanks" class="button border small rev inline">{% trans "See all supporters" %}</a>
<a href="https://hosted.weblate.org/projects/" class="button border small rev inline" style="text-decoration: none;" target="_blank">{% trans "See all projects" %}</a>
Copy link
Member

Choose a reason for hiding this comment

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

I can still see it in this place (style="text-decoration: none;").

weblate_web/templates/snippets/users.html Outdated Show resolved Hide resolved
weblate_web/templates/snippets/reviews.html Outdated Show resolved Hide resolved
weblate_web/templates/index.html Outdated Show resolved Hide resolved
Removed Inline Styling.
Copy link
Member

@nijel nijel left a comment

Choose a reason for hiding this comment

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

Thanks, we're getting closer now. Besides the things I've commented separately, can you please make the CI pass. Most of the failures are caused by formatting issues, the easiest approach to that is to install pre-commit and let it do the formatting for you. See https://docs.weblate.org/en/latest/contributing/code.html#coding-standard

Comment on lines 4172 to 4187
.container,
.footer,
.main,
.header {
width: 100%;
max-width: 1000px;
margin-left: auto;
margin-right: auto
}

@media screen and (min-width: 600px) {
.main {
display: -webkit-flex;
display: flex
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Are these styles used? I could not find single use of "main" class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the useless css.

line-height: 23px;
text-decoration: underline;
}
.unqimg{
Copy link
Member

Choose a reason for hiding this comment

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

Can you please choose a bit more descriptive class names? Neither unqimg nor contentt are easy to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay,let me commit the final version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's done

@nijel nijel mentioned this pull request Feb 1, 2021
5 tasks
@github-actions
Copy link

github-actions bot commented Feb 4, 2021

This pull request has been automatically marked as stale because it has not had any recent activity.

It will be closed soon if no further action occurs.

Thank you for your contributions!

@github-actions github-actions bot added the wontfix Nobody will work on this. label Feb 4, 2021
@nijel
Copy link
Member

nijel commented Feb 4, 2021

Stale on our side, sorry...

@nijel nijel removed the wontfix Nobody will work on this. label Feb 5, 2021
nijel added a commit that referenced this pull request Mar 1, 2021
These should be merged with #438
@nijel
Copy link
Member

nijel commented Mar 4, 2021

Rebased on current master in #537

@nijel nijel closed this Mar 4, 2021
@nijel nijel self-assigned this Mar 4, 2021
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.

2 participants