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

Feature/ubigufy #21

Merged
merged 4 commits into from
Aug 20, 2024
Merged

Feature/ubigufy #21

merged 4 commits into from
Aug 20, 2024

Conversation

AttePeramaki
Copy link

Added Icons from Material to Ubilibrary

Copy link
Member

@jlaamanen jlaamanen 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, after the mentioned fixes should be good to go!

@@ -65,20 +66,21 @@ export default function SurveyListItem(props: Props) {
}/${survey.name}`;
}, [survey.name]);

const cardStyle = survey.isPublished
Copy link
Member

Choose a reason for hiding this comment

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

Inline styles are usually bad practice, unless it's the only choice. In this case we can rely on MUI's theming and add a new class inside makeStyles, e.g. publishedCard: { ... }.

The easiest way to apply multiple conditional class names is to use the clsx utility library. It should already be installed as a 3rd party dependency, but feel free to npm install it to this project as well:

import clsx from 'clsx';

clsx(loading && classes.loading, survey.isPublished && classes.publishedCard)

@@ -0,0 +1,40 @@
import React from 'react';
import SvgIcon, { SvgIconProps } from '@mui/material/SvgIcon';
// TODO: fills...
Copy link
Member

Choose a reason for hiding this comment

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

[nit] forgotten temporary comments in this file?

/>
</g>
<defs>
<clipPath id="clip0_3398_1868">
Copy link
Member

Choose a reason for hiding this comment

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

This is the clip path I was talking about. Maybe you should check if multiple MailIcons on the same page work with this? Otherwise I guess it should be removed, but not sure if that would break this icon... 🤔


export let theme = createTheme(
{
components: {
...surveyCardOverrides,
//...textOverrides,
Copy link
Member

Choose a reason for hiding this comment

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

[nit] forgotten commented out code in this file?

}
};

//
Copy link
Member

Choose a reason for hiding this comment

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

[nit] forgotten comments?

...buttonOverrides,
...inputOverrides,
...stepperOverrides,
...textOverrides,
},
} /*
Copy link
Member

Choose a reason for hiding this comment

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

[nit] forgotten commented out code?

@AttePeramaki AttePeramaki merged commit 7128b72 into develop Aug 20, 2024
2 checks passed
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.

2 participants