[BEAM-11424-11423-11368][Website revamp] Implemented contribution guide, become a committer pages and twitter feed #13565
Conversation
…eaInternal/beam into website/contribute-twitter
|
Review: @TheNeuralBit |
|
thank you @Jakub-Sadowski ! Can you share a few screenshots of these changes? |
|
You can see it in the GCS staged output: http://apache-beam-website-pull-requests.storage.googleapis.com/13565/contribute/become-a-committer/index.html |
|
ouch - thanks Brian! |
|
No worries :) I can still review this one if that's ok with you, I'll have time this afternoon |
|
Sounds good. I'll leave it to you - but let me know if you'd like me to
review it.
…On Thu, Dec 17, 2020 at 1:47 PM Brian Hulette ***@***.***> wrote:
No worries :) I can still review this one if that's ok with you, I'll have
time this afternoon
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#13565 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJ5Z3EEL5E6DIA2RS2BWDDSVJ35FANCNFSM4U5YXXCA>
.
|
There was a problem hiding this comment.
I'm concerned that this makes a lot of changes to the actual markdown content, like website/www/site/content/en/contribute/become-a-committer.md and website/www/site/content/en/contribute/_index.md
These are exactly the sort of changes we're trying to avoid making as part of the website revamp because they're impossible to deconflict with any website updates that happen in parallel in master.
In general this PR has two types of changes in these content files:
- Changes just to make the page render correctly (e.g. adding
<div>and<br>tags) - Changes to the actual content (re-wording or re-arranging text)
We should try very hard to avoid (1), and keep the content separate from the template that renders it.
(2) should be done on the master branch, with the existing website.
If we adhere to both of those, there won't be the need to make markdown changes in the website-revamp branch
|
Review: @kgabryje |
TheNeuralBit
left a comment
TheNeuralBit
left a comment
There was a problem hiding this comment.
I just synced master into the website-revamp branch and there's now a conflict with contribute/_index.md. This is probably because of #13420 which made substantial changes to that file.
TheNeuralBit
left a comment
TheNeuralBit
left a comment
There was a problem hiding this comment.
This PR makes a lot of seemingly unnecessary changes to existing content files (blog posts, documentation, and programming guide). Some of them are benign, like changing ``` to {{< highlight >}}, but there also substantive ones like removing leading whitespace from code samples.
Even the benign ones present an issue, because they are surely going to cause merge conflicts, and they make this into an 8k line PR, which is very difficult to review. Please revert these changes.
TheNeuralBit
left a comment
TheNeuralBit
left a comment
There was a problem hiding this comment.
A few more notes here, but again, I haven't looked it all over yet because It's very challenging to review just the substantive changes amid all the changes to the code blocks.
TheNeuralBit
left a comment
TheNeuralBit
left a comment
There was a problem hiding this comment.
Thanks it looks like a lot of the extra changes have been reverted, but there are still a lot remaining. It would be great if we could keep the original {{< highlight lang >}} syntax and avoid having to change the existing content altogether.
website/www/site/content/en/documentation/transforms/python/aggregation/combineperkey.md
Show resolved
Hide resolved
TheNeuralBit
left a comment
TheNeuralBit
left a comment
There was a problem hiding this comment.
Ok a few last comments here. I think this is ready to go if we get all these addressed.
| el.value=text;document.body.appendChild(el); | ||
| el.select();document.execCommand('copy'); | ||
| document.body.removeChild(el); | ||
| alert('copied to clipboard'); |
There was a problem hiding this comment.
This alert is strange to me. It's weird to have to click "OK" after copying.
Other implementations of this sort of featureu usually pop up a notification to let you know the content has been copied, but you don't have to acknowledge it. If it's too much to address this right now let's file a jira about it.
There was a problem hiding this comment.
From fastest ways I can change it to pop up under a button, but then there would be no popover when user hover on an icon
There was a problem hiding this comment.
Ok let's just file a jira for now
Co-authored-by: Brian Hulette <hulettbh@gmail.com>
Co-authored-by: Brian Hulette <hulettbh@gmail.com>
5 participants
Implemented:
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.