fixed vertical rhythm

[Gauravjeetsingh]

Fixed Vertical rhythm of website as discussed in issue #6

[pl-semiotics]

Thank you for looking into this, I'm sure I speak for everyone in saying that we all really appreciate help with the website. I'm not sure if this fixes that issue because, among other things, I don't see any work on building all sizes in terms of base units of text size, and it looks to me like everything is still aligned based on relatively arbitrary pixel measurements.

This design also has the problem that #11 is trying to fix (I'm still waiting for an updated review on that one), in that narrow windows result in the about section text overlapping the what-is section. This is another indication that the result isn't as responsive and based off of the text size as we would like.

Also, I have not worked on the design aspect of this at all, and may be wrong, but I personally like the way that the icons on the old version bridge the tesselaction/logo screen and panel and the panel containing the "about" text blurbs.

@brlcad, @inderpreetsingh, any thoughts on this?

[Gauravjeetsingh]

Hello everyone! Thanks peter for a reviewing my work :) The font sizes are now in rem, with base font-size 18px. Thus fixing the vertical rhythm issue #6

Also I improved the markup of about section(the three circles), instead of the table layout, I shifted it to responsive div layout. That solves the issue #14. The black div(What is BRL-CAD?) has no margin-top now and is naturally placed in it's optimum place. Also the three circles are responsive.

[inderpreetsingh]

I like this pull request, but it has lots of changes. Please keep the pull requests concise, we can always add new issues for major improvements. Try to solve only one issue per pull request.

Having said that, I liked the new markup as it solves various issues. Just make sure you don't change the design a lot (like you moved the blurbs down). Keep them at their place, and if you think they would be better down, then we can discuss before finalizing it.

[inderpreetsingh]

@Gauravjeetsingh , Please also can you increase the font-sizes in your rhythm. Keep the base-font size about 21 at-least as that's what the current design is about. Again, if we want to change the design part we can discuss for sure. For what it's worth, I must say it's a great PR. Thanks!

[Gauravjeetsingh]

Hello @inderpreetsingh I am sorry to hurry all the changes in a single pull request. Will keep this in mind from now on. Working on the changes you just asked. Will commit soon. Thanks!

[Gauravjeetsingh]

@inderpreetsingh Done with the changes asked. Also, earlier I did some design changes which are removed for now.

[inderpreetsingh]

It looks good, thanks! @Gauravjeetsingh If it's not too much to ask, Can you make a list of things that this PR fixes?

[inderpreetsingh]

Hi Gaurav, I was looking into changes and found a little bug. At size 600px to 700px the words "think, invent, create" they come out of their black container. I think it would be easily fixable. Can you please look into that?

[Gauravjeetsingh]

Oh! Must have missed that. Working on it.

Also, below is the list of fixes I did in this PR

• Font sizes are now in rem with base font size 21px.
• Learn More button of about and highlights section is aligned properly at the bottom at all screen sizes.
• Fixed issue #14 by changing table markup to responsive div based grid.
• The posts section is also now responsive by replacing table layout to div grid.
• No longer misalignment of visual sparkle.

[inderpreetsingh]

Thank you, Gaurav. :+1:

[Gauravjeetsingh]

Have a look.

[inderpreetsingh]

Yeah looks good to me, @brlcad and @peter-sa , any views?

[pl-semiotics]

Looking at the rendered output of the site, this looks great to me. I have a few minor nitpicks with the code, but would be happy to merge this and then revisit them:

• [ ] A lot of elements have class names, etc. carried over from the new markup, which means that we have non-table elements with a class called table, etc. I think that this is a little confusing, and we might want to get rid of it eventually
• [ ] I'm still seeing a lot of pixel constants in the styles (for example, the 70px exactly overlap of the icons---it might be better to make it the height of one line of text, for example). In the same vein, I don't see any explanation for the rem constant values that are used in sizing other element. Unfortunately, I don't have a baseline-grid layout tool on hand, so I can't check if everything ends up properly aligned to a grid; if anyone has experience checking this kind of thing, it might be worth looking into more. However, it's still a lot better than the old version in the style department, and the improvement on the markup is big enough that I'm happy to merge: this should help greatly with being able to tweak that kind of thing without horribly breaking everything due to the ridiculous degree of nonlocality that we used to have.
• [ ] In terms of establishing a very good vertical rhythm, I think that we still need to change our text formatting to follow more typographic conventions, for example a large line height, etc. Again, this is a great step towards being able to do that.

In summary, I think that this is definitely strong enough to merge (thanks @Gauravjeetsingh!) but that we might not want to close #6 quite yet.

@brlcad, do you have any objections?

[inderpreetsingh]

@peter-sa I completely agree with you and have these points in my mind. I checked the baseline grid alignment, no it does not match perfectly. Yeah, the typographic hierarchy is bit improved, though. We still need to work on vertical rhythm.

[inderpreetsingh]

I am going ahead and merging these for the sake of improvements it offer.