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

spacing on splash page #1576

Merged
merged 3 commits into from
Sep 6, 2017
Merged

spacing on splash page #1576

merged 3 commits into from
Sep 6, 2017

Conversation

rouzbeh84
Copy link
Collaborator

extends PR #980:

I'm sure this is already known but the bug is happening because the left side is having its title put in a p tag whereas the right side is right before its equivalent p tag :(. This seems to resolve the issue for now but the markup should be parsed properly.

extends PR #980:

I'm sure this is already known but the bug is happening because the left side is having its title put in a `p` tag whereas the right side is right _before_ its equivalent `p` tag :(. This seems to resolve the issue for now but the markup should be parsed properly.
@skipjack
Copy link
Collaborator

skipjack commented Sep 5, 2017

@rouzbeh84 does this resolve #1559?

@rouzbeh84
Copy link
Collaborator Author

It does but I didn’t know if #980 was also doing other things so didn’t wanna close it out automatically. 👍🏽

@skipjack
Copy link
Collaborator

skipjack commented Sep 6, 2017

@rouzbeh84 tested this out with npm run build-test and unfortunately that change doesn't fix it in production:

image

I think the issue is that specificity is broken between development and production builds. Looking into a more stable fix now.

@skipjack
Copy link
Collaborator

skipjack commented Sep 6, 2017

This seems to resolve the issue for now but the markup should be parsed properly.

To this point, yeah this is definitely one of many indications that our markdown utility has problems. This is one thing we definitely need to address in infrastructure work from #1525. Luckily, that's one thing we actually have an agree upon plan for (webpack-contrib/webpack-defaults#73). Now the trick is just finding the time to make that move.

@skipjack
Copy link
Collaborator

skipjack commented Sep 6, 2017

I'm actually seeing some bigger discrepancies between dev and prod with markdown parsing which is odd because it should be going through the exact same process each time:

dev

image

prod

image

I know it's a little hard to tell but the pre tags are actually being rendered completely outside the homepage__right divs in prod.

Move `homepage.scss` styles to `splash.scss` and update class
names for consistency. Add the small `margin-bottom` hack to fix
the spacing issue in production.
@skipjack
Copy link
Collaborator

skipjack commented Sep 6, 2017

Ok added a hacky fix that isn't pretty but works:

image

As for a plan going forward, I think we should do this:

  • Freeze any new site features.
  • Be careful even with fixes as these can lead to unintended issues.
  • Continue content work/restructuring carefully.
  • Start really cleaning up the infrastructure of this site and hitting the things mentioned in Dev - General Updates and Fixes #1525.

And on the last point, maybe we can put the move to remark to the top of that list. I've already created a remark-loader we can use as a starting point and I bet @wooorm would help advise us through that move so we can do it in a clean way without resorting to the similar hacks that got us into trouble with marked.

cc @pierreneter @jeremenichelli

Copy link
Collaborator Author

@rouzbeh84 rouzbeh84 left a comment

Choose a reason for hiding this comment

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

they look good to me. quite an imaginative fix but it is what it is 😜 . i know you cc'd a couple other folks so i'll let them approve.

@skipjack
Copy link
Collaborator

skipjack commented Sep 6, 2017

they look good to me. quite an imaginative fix but it is what it is 😜 .

Haha yeah, I just really don't feel like debugging the markdown.js utility any more than I already have and any tweaks there could mean breaking other stuff throughout the rest of the site. A better fix would be just moving this markup to the Splash.jsx component itself but then we lose out on syntax highlighting 😢 .

If somebody has a better idea, I'm all ears but I'd rather just fix this, push the last few open content tickets forward and then start digging into the root of these problems, starting with the port from marked to remark.

i know you cc'd a couple other folks so i'll let them approve.

Just cc'd them more for a heads up on the plan part of it. Once this passes I'll "Squash & Merge" but if you or anyone else comes up with a cleaner fix I'd be happy to revert.

@rouzbeh84
Copy link
Collaborator Author

rouzbeh84 commented Sep 6, 2017

Haha yeah, I just really don't feel like debugging the markdown.js utility any more than I already have and any tweaks there could mean breaking other stuff throughout the rest of the site.

@skipjack Ah i think my comment didn't get posted proper. Thought I mentioned a dev named @jescalan who demo'd reshape at an event I organize out here in LA which is similar to remark but its html centered vs markdown and has a rich plugin ecosystem as well (plugins)

this is otherwise good to go then, approving now.

edit: just kidding, forgot this was my PR and I can't do such wizardry haha

@wooorm
Copy link

wooorm commented Sep 6, 2017

@rouzbeh84 remark is connected to rehype too through unifiedjs.

@skipjack skipjack merged commit ac57afe into master Sep 6, 2017
@skipjack skipjack deleted the hotfix/splash-code-spacing branch September 6, 2017 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants