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

Made changes to the Sound of the Salish Sea section #58

Closed
wants to merge 4 commits into from

Conversation

Saksham294
Copy link

With reference to the issue #42
I have used MUI sx props for styling the "Sounds of the Salish Sea" section of the Learn page.
I am attaching an image for the same below.
Screenshot 2022-02-04 at 6 38 17 PM

@netlify
Copy link

netlify bot commented Feb 4, 2022

✔️ Deploy Preview for orcahome ready!

🔨 Explore the source changes: 7928a12

🔍 Inspect the deploy log: https://app.netlify.com/sites/orcahome/deploys/622f831d22793a0008bc71a4

😎 Browse the preview: https://deploy-preview-58--orcahome.netlify.app

Copy link
Contributor

@evanjscallan evanjscallan left a comment

Choose a reason for hiding this comment

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

would avoid changing the Next version in the PR--recently ran into the same issue as well. Also would possibly look into theme usage for font size.

Updated next version from 12.0.10 to 12.0.9
@Saksham294
Copy link
Author

Saksham294 commented Feb 6, 2022

I am able to do the theme based styling for the heading "Sailing of the Sea". But when I try to apply to the paragraph, it is not working. I have updated the Next JS version back to original.

@Saksham294
Copy link
Author

Screenshot 2022-02-07 at 10 13 38 PM

Screenshot 2022-02-07 at 10 12 18 PM

I created themes for both heading and paragraph.
I am using ThemeProvider but it ain't working. I don't know why

@paulcretu
Copy link
Member

@Saksham294 are you still working on this PR? Is it ready to review? If so, go ahead and pull it out of draft status (mark it ready for review) and request my review

@Saksham294 Saksham294 marked this pull request as ready for review March 14, 2022 18:04
@Saksham294
Copy link
Author

@Saksham294 are you still working on this PR? Is it ready to review? If so, go ahead and pull it out of draft status (mark it ready for review) and request my review

Hey Paul I have made changes as per recommendations, like using ThemeProvider for fontSize and switching NextJS version back to original. You can review the changes made.

Copy link
Member

@paulcretu paulcretu left a comment

Choose a reason for hiding this comment

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

@Saksham294 thanks for your changes! I left a quick comment for you, but before I do a proper review can you merge in the latest changes from the main branch and fix the conflicts?

Comment on lines +17 to +30
const paragraph=createTheme();
paragraph.typography.p={
fontFamily:"Montserrat",
fontSize:'1.25rem',
fontWeight: '600'

}
const heading=createTheme();
heading.typography.h1={
fontFamily: 'Montserrat',
fontSize: '2.75rem',
fontWeight: '600'

}
Copy link
Member

Choose a reason for hiding this comment

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

This should go in the theme.ts file instead of creating new themes

Copy link
Author

Choose a reason for hiding this comment

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

Ok I have made the requested changes. I think I have an older version of the project. So should I upload the changes from that branch or create a new branch with the latest version of the project?

Copy link
Member

Choose a reason for hiding this comment

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

It's up to you how you want to do it, but what I would do is fetch/pull the latest changes into the main branch, switch to the feature branch, then run git merge main and fix the merge conflicts

Copy link
Author

Choose a reason for hiding this comment

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

I fetched the latest changes but on my master branch, so there's a conflict now. So I created another local repo and made the required changes. It is the same. Can I upload the changes from a new branch now?

Copy link
Member

Choose a reason for hiding this comment

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

That's completely up to you. It would be easiest to keep using this PR (if you're not sure how to deal with merge conflicts read this, it's an important skill to learn). But if you'd rather open a new PR that's fine too. You can also force push (git push -f) your newest code to this branch which will update this PR.

Copy link
Author

Choose a reason for hiding this comment

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

I'll open a new PR as I find it easier as of now. I'll definitely read about merge conflicts and use it in future. Thank you

Copy link
Author

Choose a reason for hiding this comment

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

I have made a new PR at the following link:#81

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, go ahead and close this PR then if everything is in the other one now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants