[SPARK-30654] Bootstrap4 docs upgrade#27369
Conversation
|
Jenkins test this please |
|
Test build #117486 has finished for PR 27369 at commit
|
|
I haven't forgotten about this, just maybe waiting to merge big changes in master until after 3.0 |
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
|
@srowen are you able to remove the stale tag to prevent this from being automatically closed until you have a chance to review the commit post Spark 3.0 release work? |
|
Looks pretty reasonable to go for at this point. Still up to date and worth me checking the docs build now? |
srowen
left a comment
srowen
left a comment
There was a problem hiding this comment.
Looks good! I built the docs and all looks OK. Just one minor question.
docs/_layouts/global.html
Outdated
| <link rel="stylesheet" href="css/bootstrap.min.css"> | ||
| <style> | ||
| body { | ||
| {% if page.url contains "/ml" or page.url contains "/sql" or page.url contains "migration-guide.html" %} |
There was a problem hiding this comment.
I built the docs to take a look, and wondered, why special case these? the /ml and /sql pages look like they have too little padding at the top.
There was a problem hiding this comment.
@clarkead I'd even merge this as-is but wondering if the above change is necessary?
There was a problem hiding this comment.
Thank you for reviewing this. I believe it was trying to address an issue with the left nav on those pages, but I don't remember the specifics. I do see the spacing difference though and I agree we should try to fix it. There should be a way to do this without making an exception as this does. Thank you for calling it out. I appreciate the feedback.
In reviewing this I think I might have noticed a display bug on the rdd-programming-guide.html and structured-streaming-programming-guide.html pages when viewed in a mobile device's browser.
Let me take a look at both issues and fix them. I'll try to submit another update before the end of this week.
There was a problem hiding this comment.
It looked OK to me otherwise. Maybe it is just a matter of removing the special-casing here.
There was a problem hiding this comment.
@clarkead I think it's sufficient (for now) to just remove the special casing code here. It seemed like it was causing the differences, itself, unless I really misunderstand its other effects - does something else go wrong if you don't special case? it seems to raise the title too far, and that's all I'd expect, so removing it would fix it.
There was a problem hiding this comment.
@srowen Sorry about the delay getting an updated PR. I've pushed two small tweaks to address the items in this thread. The first is to try and fix the formatting on those pages with left navigation that had special logic. I've removed the logic and fixed the width on the expand/collapse trigger.
The second change is with regards to the issue I saw on the rdd-programming-guide.html and streaming-programming-guide.html pages. It ended up being an issue where older versions of Bootstrap where setting the max-width on images to 100%, but Bootstrap4 removed that. As a result larger image files were blowing out the right side. I've added a css style to re-add this in main.css.
viirya
left a comment
viirya
left a comment
There was a problem hiding this comment.
Thanks for working on this. I don't look at the details, but am wondering the file size change, e.g., bootstrap.css was removed (5624 lines), but boostrap.min.css only changed few lines. Is it because we don't use bootstrap.css at all previously and we only need boostrap.min.css?
|
The minified version is indeed smaller (good, we should have done that already). The line count is misleading though as the minified version also omits almost all line breaks. They are very long lines :) |
…er versions of Bootstrap did this by default.
|
Jenkins test this please |
|
Looks good to me pending tests. Here's a screen shot of the new main docs page, for example, which shows it's fine and almost identical except for slightly different fonts (looks nicer IMHO).
|
|
Test build #127938 has finished for PR 27369 at commit
|
|
Merged to master |
|
@srowen Thank you for your patience and help getting this merged |
5 participants
What changes were proposed in this pull request?
We are using an older version of Bootstrap (v. 2.1.0) for the online documentation site. Bootstrap 2.x was moved to EOL in Aug 2013 and Bootstrap 3.x was moved to EOL in July 2019 (https://github.com/twbs/release). Older versions of Bootstrap are also getting flagged in security scans for various CVEs:
I haven't validated each CVE, but it would probably be good practice to resolve any potential issues and get on a supported release.
The bad news is that there have been quite a few changes between Bootstrap 2 and Bootstrap 4. I've tried updating the library, refactoring/tweaking the CSS and JS to maintain a similar appearance and functionality, and testing the documentation. This is a fairly large change so I'm sure additional testing and fixes will be needed.
How was this patch tested?
This has been manually tested, but as there is a lot of documentation it is possible issues were missed. Additional testing and feedback is welcomed. If it appears a whole section was missed let me know and I'll take a pass at addressing that section.