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

fix: broken links and HTML validation #455

Merged
merged 1 commit into from Aug 13, 2020

Conversation

zregvart
Copy link
Member

This fixes broken link to component reference and outstanding HTML
validation issues.

@zregvart
Copy link
Member Author

@aashnajena and @AemieJ can you have a look at this and see if I missed something?

@@ -114,6 +114,12 @@ section.frontpage.columns.blog-posts {
background-color: var(--color-smoke-50);
}

section.frontpage.blog-posts ul {
display: flex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
display: flex;
display: flex;
flex-wrap: wrap;

Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems problematic.. This is how it looks for me:

Screenshot 2020-08-12 at 1 27 16 PM

I've suggested another change to make sure the flex-direction of the blog-posts section becomes 'column' for <1023px.

@AemieJ
Copy link
Contributor

AemieJ commented Aug 12, 2020

In addition to it, the text isn't aligned with the icon within projects section for the frontpage in the mobile view.
Add the media screen code in addition to the previous code for frontpage content.

section.frontpage.columns.functionalities h2,
section.frontpage.projects .project .content h2,
section.frontpage.projects .project .content p {
  text-align: left;
  text-transform: none;
 @media screen and (max-width: 626px) {
      text-align: center;
  }
}

Copy link
Contributor

@aashnajena aashnajena left a comment

Choose a reason for hiding this comment

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

We need to scope some rules to center-align the "projects" in the mobile view. The rules are already present, they're just not scoped correctly.

line 364 of frontpage.css reads :

  section.frontpage.projects .project h2,
  section.frontpage.projects .project .icon,
  section.frontpage.columns.functionalities .box .content h2,
  section.frontpage.columns.functionalities .box .content,
  section.frontpage.columns.functionalities .box .icon,
  section.frontpage.columns.functionalities p,
  section.frontpage.projects p {
    text-align: center;
    margin: 0;
    width: 100%;
  }

We must change it to:

  section.frontpage.projects .project .content h2,
  section.frontpage.projects .project .icon,
  section.frontpage.columns.functionalities .box .content h2,
  section.frontpage.columns.functionalities .box .content,
  section.frontpage.columns.functionalities .box .icon,
  section.frontpage.columns.functionalities p,
  section.frontpage.projects .content p {
    text-align: center;
    margin: 0;
    width: 100%;
  }

Also, we have to make sure there's some space below the project icons.

Line 301 of frontpage.css reads:

section.frontpage.functionalities .icon img {
    max-width: 25vw;
    margin: 2vw;
  }

We must change it to:

section.frontpage.functionalities .icon img, 
section.frontpage.projects .icon img {
    max-width: 25vw;
    margin: 2vw;
  }

@aashnajena
Copy link
Contributor

The second problem I see is that blog-posts are not getting aligned as a column at <1023px. To amend this, line 287 of frontpage.css reads:

section.frontpage.columns.blog-posts {
    flex-direction: column;
  }

We must change it to:

section.frontpage.columns.blog-posts ul {
    flex-direction: column;
  }

@zregvart
Copy link
Member Author

Thanks! I’ll make the changes a bit later today.

@zregvart
Copy link
Member Author

I tried to include all suggestions. I did found that using padding rather than margin for the list of blog posts works better with sizing. I also found border-radius directive on a element without a border (I think we have several of those, we're going to need to clean that up a bit). Hopefully this is better now. Can you have another look @aashnajena and @AemieJ. Thanks!

@@ -97,7 +97,6 @@ div.frontpage.news {
text-align: center;
margin-left: 3rem;
background-color: var(--color-smoke-50);
border-radius: 1rem 1rem 0 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This border radius is for for the "what's new" title div on desktop view. I think we can keep this..

@AemieJ
Copy link
Contributor

AemieJ commented Aug 13, 2020

This needs to be applied yet as the content of the project and button are left-aligned within the mobile screen.

Copy link
Contributor

@aashnajena aashnajena left a comment

Choose a reason for hiding this comment

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

Apart from this small change, everything looks fine to me.

section.frontpage.projects .project .icon,
section.frontpage.columns.functionalities .box .content h2,
section.frontpage.columns.functionalities .box .content,
section.frontpage.columns.functionalities .box .icon,
section.frontpage.columns.functionalities p,
section.frontpage.projects p {
section.frontpage.projects .content p {
Copy link
Contributor

Choose a reason for hiding this comment

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

@AemieJ instead of adding a media-code snippet, this line should to be scoped to section.frontpage.projects .project .content p. It's getting overwritten by the rule on line 197. Sorry for missing it out before.

section.frontpage.projects .project .icon,
section.frontpage.columns.functionalities .box .content h2,
section.frontpage.columns.functionalities .box .content,
section.frontpage.columns.functionalities .box .icon,
section.frontpage.columns.functionalities p,
section.frontpage.projects p {
section.frontpage.projects .content p {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
section.frontpage.projects .content p {
section.frontpage.projects .project .content p {

@@ -56,7 +56,7 @@ Apache Camel is standalone, and can be embedded as a library within Spring Boot,
## Packed with Components
Packed with several hundred components that are used to access databases, message queues, APIs or basically anything under the sun. Helping you integrate with everything.

<p><a class="button-dark" href="/components/latest">Go to Component Reference</a></p>
<p><a class="button-dark" href="/components/latest/">Go to Component Reference</a></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I dared to take the link fixes to #456 so that we can publish Quarkus 1.0.0 announcement.

This fixes broken link to component reference and outstanding HTML
validation issues.
@zregvart
Copy link
Member Author

We need to get this merged to successfully build the website. Thanks for the review and suggestions @aashnajena and @AemieJ. If there are more issues with this please create followup PRs.

@zregvart zregvart merged commit 97cec7b into apache:master Aug 13, 2020
@zregvart zregvart deleted the pr/fix-validation branch August 13, 2020 21:30
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.

None yet

4 participants