Used ThemeProvider for fontSize #81
Conversation
✅ Deploy Preview for orcahome ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
@Saksham294 thanks for the changes! Left you some comments. Also it looks like CI is failing, take a look at the checks. You should probably run npm run lint and npm run format, also make sure you've done npm install because it seems like you might not have all husky and lint-staged installed
I did "npm install' and it stated "husky-git hooks installed, changed 3 packages" . However I didn't understand what went wrong. Like I just updated files : learn.jsx,theme.ts,Learn.module.css. There was nothing related to lint, format or husky files. I also don't know anything about these files. |
|
You probably started working on this feature using an old version of the project, before I set up So to fix it, you should run Also, as an extra tip, if you're using VS Code, take a look at the |
|
I think now it is fine. Can you check? Thanks a lot for all the support |
There was a problem hiding this comment.
The checks look good, thanks for the changes! Looks like you removed settings.default.json, I re-added it (others will want it too). Also if you're not using a CSS class, might as well remove it. I went ahead and just made the changes because they're minor. However, I don't think this is ready to merge yet, left you some more comments
src/styles/theme.ts
Outdated
| h1: { | ||
| fontFamily: 'Mukta', | ||
| fontSize: '40px', | ||
| fontWeight: '600', | ||
| }, | ||
| body1: { | ||
| fontFamily: 'Montserrat', | ||
| fontSize: '20px', | ||
| fontWeight: '600', | ||
| }, |
There was a problem hiding this comment.
I think these font sizes are way too large. Just take a look at the designs
vs how this PR looks on my screen
I think we should probably just leave this as it was before because it was close enough, and we can make these kind of adjustments later on (changing the font sizes here changes it for the entire site)
There was a problem hiding this comment.
I used the Container component and put the entire section within that container. Here's an image of the same, is it fine?
There was a problem hiding this comment.
Though the design is a bit different from the one on the Figma page. Earlier I tried to match values of the styles as mentioned on the Figma design page.Like margin of the paragraph from the image was 'x' px on the Figma design so I copied the same. But it looks different with this PR as you mentioned above.
There was a problem hiding this comment.
The section size is not like before, it is somewhat smaller in my opinion, while using the Container component. I don't know if it is optimum . I put the header, paragraph and the image inside one single Container component. I tried using the maxWidth and sx props of the Container component but no success. Am I doing something wrong?
There was a problem hiding this comment.
I wrapped this section in a Container component and added individual font sizes for the text using the sx prop(removed the fontSize from the theme.ts file). I think the size is small even though I used the maxWidth="xl". I also removed marginLeft from Box components and applied it to just the Container component. Can you check if it is fine now?
|
UI checks pass. Does this PR also cover the interactive graphic? If so, please link "Sounds of the Salish Sea" to this URL: https://orcasound.net/ed/booth/local.html?learn |
|
We are going to deploy all passed PRs this Tuesday. Would it be possible for you to pass this PR so it's ready for our deployment session on Tuesday, Oct. 17? |
With reference to the PR #58
I have used MUI sx props for styling the "Sounds of the Salish Sea" section of the Learn page.