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

feat: extract various components #868

Merged
merged 22 commits into from
Feb 28, 2022
Merged

feat: extract various components #868

merged 22 commits into from
Feb 28, 2022

Conversation

shaleynikov
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Feb 18, 2022

Codecov Report

Merging #868 (130f634) into main (a9daf36) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #868      +/-   ##
==========================================
+ Coverage   66.09%   66.15%   +0.06%     
==========================================
  Files          86       86              
  Lines        3120     3122       +2     
  Branches      611      613       +2     
==========================================
+ Hits         2062     2065       +3     
+ Misses       1026     1025       -1     
  Partials       32       32              
Impacted Files Coverage Δ
webapp/javascript/ui/Sidebar.scss 30.77% <ø> (ø)
webapp/javascript/components/Sidebar.tsx 78.75% <100.00%> (+1.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9daf36...130f634. Read the comment docs.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 18, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 464.09 KB (+4.03% 🔺) 9.3 s (+4.03% 🔺) 969 ms (-14.71% 🔽) 10.3 s

@eh-am
Copy link
Collaborator

eh-am commented Feb 18, 2022

/create-server

onChange={handleChange}
/>
<InputField
label="Conform new password"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
label="Conform new password"
label="Confirm new password"

[styles.success]: type === 'success',
})}
>
{children}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we call it message? Children to me conveys any arbitrary component tree.

Comment on lines 15 to 16
[styles.error]: type === 'error',
[styles.success]: type === 'success',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This kinda implies it's possible to have both styles.error and styles.success, which is obviously untrue.

Can we rewrite as a pure function?

const  getClassnameForType = () => {
  switch (type) {
    case 'error': return styles.error;
    case 'success': return styles.success;
  }
}

Comment on lines 3 to 16
function confirmDelete(object: string, onConfirm) {
Swal.fire({
title: `Are you sure you want to delete ${object}?`,
showCancelButton: true,
confirmButtonText: 'Delete',
backdrop: true,
allowOutsideClick: true,
confirmButtonColor: '#dc3545',
}).then((result) => {
if (result.isConfirmed) {
onConfirm();
}
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feels like we are missing one more abstraction: a wrapper around sweetalert2.

That way people don't have to interact with sweetalert2 directly, and if necessary we can replace for something else later.

It would be something more or less like this
https://github.com/pyroscope-io/pyroscope/blob/main/webapp/javascript/ui/Notifications.tsx#L20-L52

@shaleynikov shaleynikov marked this pull request as ready for review February 25, 2022 15:54
@petethepig petethepig merged commit fb4c2fc into main Feb 28, 2022
@petethepig petethepig deleted the fix/components branch February 28, 2022 21:22
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.

None yet

4 participants