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

Upgrade frontend packages #917

Merged
merged 24 commits into from Jan 23, 2024
Merged

Upgrade frontend packages #917

merged 24 commits into from Jan 23, 2024

Conversation

auloin
Copy link
Contributor

@auloin auloin commented Jan 18, 2024

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jan 18, 2024
@auloin auloin marked this pull request as ready for review January 19, 2024 17:10
@auloin auloin requested a review from rdehuyss January 19, 2024 17:10
Copy link
Contributor

@rdehuyss rdehuyss 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. I love the StatCard and think we should use a similar approach for the problems and the different state components.
I also have the impression that the overall font-size is smaller. Is that possible or is my eyesight getting worse 🤓?

I also noticed that if there are 2 warnings and an error in the dashboard and I dismiss only one warning, the 2 of them are gone. I also think the error should go on top.

I would also like to add eslint and prettier. However, since trying it, Intellij consumes all my CPU

Comment on lines 10 to 14
const classes = {
alert: {
fontSize: '1rem'
}
}));
};
Copy link
Contributor

Choose a reason for hiding this comment

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

When do we use inline styles (like job-code) vs classes (like here)?

Copy link
Contributor

Choose a reason for hiding this comment

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

See also my later comment about style duplication of the problem cards. What's your opinion on this?

Comment on lines 5 to 11
const classes = {
footer: {
paddingTop: '1rem',
width: '100%',
display: 'inline-block'
}
}));
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we inline this or not? See earlier comment about consistency.

@auloin
Copy link
Contributor Author

auloin commented Jan 22, 2024

I also have the impression that the overall font-size is smaller. Is that possible or is my eyesight getting worse 🤓?

I checked with chrome font-sizes are still the same.

I also noticed that if there are 2 warnings and an error in the dashboard and I dismiss only one warning, the 2 of them are gone. I also think the error should go on top.

Should be fixed.

@@ -148,7 +146,7 @@ const JobView = (props) => {
const mustGoBack = 'delete' === apiStatus.type;
setApiStatus(null);
if (mustGoBack) {
history.goBack();
navigate(-1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this still necessary?

@rdehuyss rdehuyss merged commit 1c38da6 into v7 Jan 23, 2024
2 checks passed
@auloin auloin deleted the build/update-frontend-packages branch January 23, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants