-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[BEAM-11372-11371-11369][Website revamp] Implemented community and contact us pages, navbar on mobiles #13511
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
[BEAM-11372-11371-11369][Website revamp] Implemented community and contact us pages, navbar on mobiles #13511
Conversation
|
Review: @TheNeuralBit |
|
thank you @Jakub-Sadowski ! Can you share a few screenshots of these changes? |
|
|
Retest this please |
|
Review: @kgabryje |
TheNeuralBit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments here.
Note this PR has the same issue as #13565, there are changes to the content files that will start to conflict with any changes made on master. If we start to diverge it will be very difficult to make sure we don't lose content changes made on master. It's even worse than the hugo upgrade - in that case we were able to at least diff the generated html since it should be identical. We won't have that option in this case since the style/layout is changing too.
Co-authored-by: Brian Hulette <hulettbh@gmail.com>
| | [JIRA bug tracker](https://issues.apache.org/jira/browse/BEAM) | Report bugs / discover known issues | | ||
| | [StackOverflow](https://stackoverflow.com/questions/tagged/apache-beam) | Ask and answer user support questions | | ||
| | [Slack](https://s.apache.org/beam-slack-channel) | Chat with users and developers in the ASF Slack. Note: Please [join the #beam channel](https://s.apache.org/beam-slack-channel) after you [created an account](https://s.apache.org/slack-invite). Please do not ask Beam questions in #general. | | ||
| Here should be a short description on what mailing lists are and how to use them. Also information on how does subscribe, unsubscribe and archives do work (blank email etc). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like placeholder copy. Do you actually want to merge this now? When will it be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We got some new copies and I just pushed this site with new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! @griscz could you review the copy changes in this PR to make sure they match your expectations? The changes should all be in the .md and .yaml files
…ub.com/PolideaInternal/beam into website/community-contact-pages-navbar
Co-authored-by: Brian Hulette <hulettbh@gmail.com>
…ub.com/PolideaInternal/beam into website/community-contact-pages-navbar
TheNeuralBit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok the HTML and CSS changes LGTM aside from a few more comments. Thanks for making all the fixes!
I don't feel great about the new content though, much of it seems a little unprofessional, or has grammatical errors.
@griscz has the copy already been reviewed?
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| - title: <a href="community/join-beam">Join Beam Community</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be preferable if there were a separate attribute for the link, and the template handled making it into an <a> tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I changed it
|
One more thing - the header is still rendering strangely for me when I view the staged website: This is on my Linux desktop with the browser fullscreen. Is there anything I can do to help you debug this problem if you can't reproduce it? |
Co-authored-by: Brian Hulette <hulettbh@gmail.com>
It's the problem I was talking about lastly, is hard to precisely cut parts of code which are needed, so during fixes to mobile navbar, navbar on desktop was changed also but some styles needed were absent, now I added them too and it looks fine |
|
Run Java PreCommit |
|
Run Java_Examples_Dataflow PreCommit |
|
Run Java_Examples_Dataflow_Java11 PreCommit |
|
Run Spotless PreCommit |
TheNeuralBit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created #13867 where I duplicated this PR, but merged in website-revamp to clean up the noise. Based on a review over there this still looks good, except I still think we need to update the copy for the community and contact us pages.
TheNeuralBit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Jakub-Sadowski. If you want to go ahead and get this merged before we get the updated copy from @griscz, please add TODOs to indicate which copy still needs to be updated. That way we won't accidentally merge some temp/placeholder copy to master.
I will get the list with missing things in the morning and all TODOs |
TheNeuralBit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the updated copy! It looks like there are a couple of files with copy that didn't get updated though, are you still waiting on copy for these?
Co-authored-by: Brian Hulette <hulettbh@gmail.com>
TheNeuralBit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @Jakub-Sadowski






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.