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

replaced html with mui in index #1748

Closed
wants to merge 5 commits into from

Conversation

nora-zajzon
Copy link
Member

Fixes #1679

What changes did you make and why did you make them ?

  • Changed all the HTML components to MUI applicable components
  • Used css files to format instead of inline sx to style like @jbubar requested. Caused some visual changes in font and what not. Let me know if this is an issue

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Visuals before changes are applied Screenshot 2024-09-17 at 11 04 02 AM
Visuals after changes are applied Screenshot 2024-09-17 at 10 57 45 AM

@nora-zajzon
Copy link
Member Author

@JackHaeg I made the requested changes for this pr here not sure if i can close the other or if it still has pending merges

@JackHaeg
Copy link
Member

@nora-zajzon Thank you for making those changes to this PR! Can you please comment directly on the other PR you are referencing above, tag Trillium, and ask him if you can close it?

Copy link
Member

@trillium trillium left a comment

Choose a reason for hiding this comment

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

Hey @nora-zajzon ,

I'm taking a look at this PR now, thanks for ding a solid tarnsition of the components to MUI. I know that bringing in other MUI components comes with additional challenges in the CSS becuase MUI has some prepackaged styling, and we can solve that in a few ways.

Ideally our normal button styles are normalized across the app, but I don't think that is true for the stats page.

I took a look at the CSS changes, and if we can at all help it I'd prefer if we don't use !important in our codebase. Ideally we'd use a more specific selector like .parent-class .current-class or button.current-class to ensure that the CSS is applied correctly. That way if another dev comes along later and needs to override something they don't have to combat the absolute speicifcity that !important requires.

Can you please go through these classes and

  • !important
  • Make each selector more specific to override MUI styles instead

CSS can be finicky, so if you need help with this please reach out.

Thanks!

Screenshots

Current on dev and prod

image

With !important

image

Without !important

image

Copy link
Member

@jbubar jbubar left a comment

Choose a reason for hiding this comment

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

Requesting to redo this without !important

Copy link
Member

@jbubar jbubar left a comment

Choose a reason for hiding this comment

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

Requesting to redo this without !important

@JackHaeg
Copy link
Member

@nora-zajzon & @trillium - FYI, it looks like there are a number of other commits that were added to this PR, which have already been merged (e.g., "Create CODE_OF_CONDUCT.md"). Trillium - Nora has provided some more context in the Slack conversation between the 3 of us. Please let her know what next steps to take and how to undo her last push.

@nora-zajzon
Copy link
Member Author

nora-zajzon commented Sep 25, 2024

Newest commit to this was re done here. I am planning to close this.

@JackHaeg
Copy link
Member

JackHaeg commented Oct 1, 2024

@nora-zajzon In your last comment above, you mentioned that the "Newest commit to this was re done #1748", which links to the same page. I assume the latest commit was done here: #1706

  • Is that correct?
  • Also, am I right to assume you are still planning to close this PR?

@JackHaeg
Copy link
Member

JackHaeg commented Oct 1, 2024

Closing PR, as new PR has been created here: #1706

@JackHaeg JackHaeg closed this Oct 1, 2024
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.

Update HTML components to MUI: ./src/components/admin/reports/index.js
5 participants