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.

Sign up for GitHub

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

Jump to bottom

refactor: [M3-6268] - MUI v5 Migration SRC > Features > Help #9408

Merged
merged 25 commits into from
Aug 2, 2023
Merged

refactor: [M3-6268] - MUI v5 Migration SRC > Features > Help #9408

merged 25 commits into from
Aug 2, 2023

Conversation

carrillo-erik
Copy link
Contributor

@carrillo-erik carrillo-erik commented Jul 17, 2023 •
edited
Loading

Description 📝

MUI v5 Migration for SRC > Features > Help

Major Changes 🔄

  • Ran tss-react codemod on SRC > Features > Help
  • Replaced createStyles() with makeStyles() and styled() API
  • Replaced default exports with named exports.
  • Corrected function name in HelpResources file to match file name.

How to test 🧪

  1. Verify the functionality around the Help & Support landing page works as expected.
  2. Look for any UI defects or regressions in the Help & Support landing page.

@carrillo-erik carrillo-erik self-assigned this Jul 17, 2023
@mjac0bs mjac0bs self-requested a review July 17, 2023 16:19
mjac0bs
mjac0bs reviewed Jul 17, 2023
View reviewed changes
Copy link
Contributor

@mjac0bs mjac0bs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

These pages were mostly looking good to me - there's one regression I noted below with the popular posts.

I also noticed a couple of styling fixes we could make here, optionally, because they're present in prod:

Add some spacing to the left side of the page at screen width959 on down - this feels unnecessarily tight.
Screenshot 2023-07-17 at 12 49 00 PM

Change the heading about the search bar ("What can we help you with?") from black text to white text. All the other section headings on the page are white and it seems odd that this is black; I don't remember if it has always been like this or if it is an intentional design choice (and if it is intentional, I am curious about the reason behind it).

Screenshot 2023-07-17 at 12 32 47 PM

@carrillo-erik
Copy link
Contributor Author

carrillo-erik commented Jul 18, 2023 •
edited
Loading

@mjac0bs Both of your suggestions make sense and I went ahead and implemented them with my latest changes. Let me know how you feel about them.

@carrillo-erik carrillo-erik requested a review from mjac0bs July 18, 2023 14:31
mjac0bs
mjac0bs reviewed Jul 18, 2023
View reviewed changes
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Confirmed the regression is fixed, thank you! White header text LGTM and I left one more comment on the spacing issue.

packages/manager/src/features/Help/Panels/SearchPanel.tsx Outdated
@@ -36,7 +36,7 @@ const StyledRootContainer = styled(Paper, {
const StyledH1Header = styled(H1Header, {
label: 'StyledH1Header',
})(({ theme }) => ({
color: theme.color.white,
color: 'white',
Copy link
Contributor

Choose a reason for hiding this comment

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

The white text looks better to me. Someday we'll fix our theme colors so that theme.color.white is not black in dark mode. 😅

jaalah-akamai
jaalah-akamai reviewed Jul 24, 2023
View reviewed changes
jdamore-linode
jdamore-linode reviewed Jul 27, 2023
View reviewed changes
packages/manager/src/components/H1Header/H1Header.tsx Outdated
@@ -34,7 +27,7 @@ export const H1Header = (props: H1HeaderProps) => {
}}
className={className}
component={renderAsSecondary ? 'h2' : 'h1'}
data-qa-header={dataQaEl ? dataQaEl : ''}
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll probably want to keep the data-qa-header attribute and its value the way it is since it's used a lot throughout the Cypress tests.

The general convention here is that the data-testid attributes are used mainly by our Jest tests to select elements (e.g. via getByTestId and similar React Testing Library helpers), whereas the data-qa-* attributes are used mainly by our Cypress tests both to select elements and occasionally to use its value in some way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted this change and updated the test as well.

cpathipa
cpathipa reviewed Aug 1, 2023
View reviewed changes
Copy link
Contributor

@cpathipa cpathipa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! I left comments to remove using usage of makeStyles for some components that have simple styles.

packages/manager/src/features/Help/Panels/SearchPanel.tsx
@@ -36,7 +36,7 @@ const StyledRootContainer = styled(Paper, {
const StyledH1Header = styled(H1Header, {
label: 'StyledH1Header',
})(({ theme }) => ({
color: theme.color.white,
color: theme.name === 'dark' ? theme.color.black : theme.color.white,
Copy link
Contributor

Choose a reason for hiding this comment

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

Font white make sense to me in dark mode, But, do we have UX approval for this?

local prod
image image

jaalah-akamai
jaalah-akamai approved these changes Aug 2, 2023
View reviewed changes
Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for making changes

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! Add'tl Approval Needed Waiting on another approval! and removed Add'tl Approval Needed Waiting on another approval! Approved Multiple approvals and ready to merge! labels Aug 2, 2023
cpathipa
cpathipa approved these changes Aug 2, 2023
View reviewed changes
Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

Nice work! @carrillo-erik Thanks making the changes.

@carrillo-erik carrillo-erik merged commit ecc5cd0 into linode:develop Aug 2, 2023
jaalah-akamai pushed a commit that referenced this pull request Aug 7, 2023
@carrillo-erik @jaalah
refactor: [M3-6268] - MUI v5 Migration SRC > Features > Help (#9408)
7fa5597 
* refactor: [M3-6268] - Apply previous work

* Remove redundant index file

* Add changeset

* Fix issues from PR feedback

* Implement new styling api and remove unused css

* Styled components implementation

* Convert class comp to funct comp and rewrite tests

* Remove dead code from PR feedback

* Fix error with search results

* Debounce searchAlgolia func and minor clean up

* Replace useMemo with useCallback

* Remove old styles in favor of using sx prop

* Replace old styles in favor of sx prop

* Remove comments and default export
jaalah-akamai pushed a commit that referenced this pull request Aug 7, 2023
@carrillo-erik @jaalah
refactor: [M3-6268] - MUI v5 Migration SRC > Features > Help (#9408)
a6f3485 
* refactor: [M3-6268] - Apply previous work

* Remove redundant index file

* Add changeset

* Fix issues from PR feedback

* Implement new styling api and remove unused css

* Styled components implementation

* Convert class comp to funct comp and rewrite tests

* Remove dead code from PR feedback

* Fix error with search results

* Debounce searchAlgolia func and minor clean up

* Replace useMemo with useCallback

* Remove old styles in favor of using sx prop

* Replace old styles in favor of sx prop

* Remove comments and default export
@mjac0bs mjac0bs mentioned this pull request Aug 7, 2023
Merged
jaalah-akamai pushed a commit that referenced this pull request Aug 8, 2023
@carrillo-erik @jaalah
refactor: [M3-6268] - MUI v5 Migration SRC > Features > Help (#9408)
c29ac7e 
* refactor: [M3-6268] - Apply previous work

* Remove redundant index file

* Add changeset

* Fix issues from PR feedback

* Implement new styling api and remove unused css

* Styled components implementation

* Convert class comp to funct comp and rewrite tests

* Remove dead code from PR feedback

* Fix error with search results

* Debounce searchAlgolia func and minor clean up

* Replace useMemo with useCallback

* Remove old styles in favor of using sx prop

* Replace old styles in favor of sx prop

* Remove comments and default export
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjac0bs mjac0bs mjac0bs approved these changes

@cpathipa cpathipa cpathipa approved these changes

@jaalah-akamai jaalah-akamai jaalah-akamai approved these changes

@jdamore-linode jdamore-linode Awaiting requested review from jdamore-linode

Assignees

@carrillo-erik carrillo-erik

Labels
Add'tl Approval Needed Waiting on another approval!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@carrillo-erik @jdamore-linode @mjac0bs @cpathipa @jaalah-akamai @bnussman-akamai