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

Coding: Develop a timeline for programs page (#73) #150

Merged
merged 31 commits into from
Jan 31, 2021

Conversation

iamkryptonite
Copy link
Contributor

@iamkryptonite iamkryptonite commented Oct 19, 2020

Description

Developed a timeline for the events page.

Fixes #73

Type of Change:

Delete irrelevant options.

  • Code
  • User Interface

Code/Quality Assurance Only

  • Bugfix (a non-breaking change that fixes an issue)
  • This change requires a documentation update (software upgrade on readme file)
  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Describe the tests you ran to verify your changes. Provide instructions or GIFs so we can reproduce. List any relevant details for your test.
AnitaB_org_open_source

ezgif com-video-to-gif

gh-pages Link: https://iamkryptonite.github.io/anitab-org.github.io/

Checklist:

Delete irrelevant options.

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented on my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been published in downstream modules

@annabauza
Copy link
Contributor

this looks great we just need to tweak the code a bit

@Akanksha1212 Akanksha1212 added the Status: Needs Review PR needs an additional review or a maintainer's review. label Oct 31, 2020
@iamkryptonite
Copy link
Contributor Author

Hey @annabauza I have made the necessary changes in the code ,could you please review it once

@nandini45
Copy link
Member

@Bucky25 can you review this one?

})
export const Months = styled(View,{
marginBottom:'15px',
Copy link
Contributor

Choose a reason for hiding this comment

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

please make it power of 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

Comment on lines 63 to 64
marginBottom:'10px',
Copy link
Contributor

Choose a reason for hiding this comment

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

please make it power of 2

Copy link
Contributor

Choose a reason for hiding this comment

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

and please add a link to your deployed gh pages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@annabauza annabauza left a comment

Choose a reason for hiding this comment

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

Generally very good code. I have left you mostly notes related to css. Thanks.

@iamkryptonite
Copy link
Contributor Author

iamkryptonite commented Dec 28, 2020

Hi, it looks like the PR will work only for the web version. We should not use things like classes and jQuery in React Native. Please do these changes.
Thanks

Hi,I have made the changes. Please review once. @tichnas

@tichnas
Copy link
Contributor

tichnas commented Dec 28, 2020

@iamkryptonite I think you're still using jQuery and pixels which don't go well with React Native. Can you please change them also?
Thanks for giving time to do these changes :)

@iamkryptonite
Copy link
Contributor Author

@iamkryptonite I think you're still using jQuery and pixels which don't go well with React Native. Can you please change them also?
Thanks for giving time to do these changes :)

Hi @tichnas, I have removed jquery and used useRef instead. And I have also changed all the places where pixels were used.

Copy link
Contributor

@annabauza annabauza left a comment

Choose a reason for hiding this comment

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

💯 This is great thank you!

@annabauza
Copy link
Contributor

@iamkryptonite please call yarn in command line and commit yarn.lock because travis is failing.

Copy link
Contributor

@tichnas tichnas left a comment

Choose a reason for hiding this comment

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

Please commit the changes to yarn.lock (and don't delete the file)
Also, remove your homepage link from package.json and can you please tell what's @babel/plugin-transform-react-jsx used for?

Thanks

@iamkryptonite
Copy link
Contributor Author

Please commit the changes to yarn.lock (and don't delete the file)
Also, remove your homepage link from package.json and can you please tell what's @babel/plugin-transform-react-jsx used for?

Thanks

Hey, I have made the requested changes and also removed the babel package as it was not required.

Copy link
Contributor

@annabauza annabauza left a comment

Choose a reason for hiding this comment

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

please resolve conflict and merge with existing events tab. Thanks!

@annabauza annabauza added Status: Changes Requested Changes are required to be done by the PR author. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Jan 17, 2021
Copy link
Contributor

@annabauza annabauza left a comment

Choose a reason for hiding this comment

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

Generally OK for first iteration just a quick question does homepage change would not break github static pages?

package.json Outdated
@@ -1,11 +1,13 @@
{
"homepage": "./",
"homepage": "/",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not really sure, I think this was a mistake so have changed it.

Copy link

@Vishal-raj-1 Vishal-raj-1 left a comment

Choose a reason for hiding this comment

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

Great Work @iamkryptonite !! Could you please squash your commits

@annabauza
Copy link
Contributor

@Vishal-raj-1 we are squashing comments on merge, no need to do that.

@annabauza annabauza merged commit a26b10c into anitab-org:develop Jan 31, 2021
iamkryptonite added a commit to iamkryptonite/anitab-org.github.io that referenced this pull request Jan 31, 2021
@iamkryptonite
Copy link
Contributor Author

Hey @annabauza there is one small change that I need to make in my code.

@annabauza
Copy link
Contributor

Rise the issue and ask to be assigned

@iamkryptonite
Copy link
Contributor Author

Rise the issue and ask to be assigned

Hey @annabauza I have raised the issue. Could you pls approve it and add relevant labels.
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Changes Requested Changes are required to be done by the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coding: Develop a timeline of recent programs for the Programs Page
7 participants