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

Add DownloadImageModal component #17

Merged
merged 1 commit into from
Dec 13, 2023
Merged

Conversation

JSReds
Copy link
Contributor

@JSReds JSReds commented Sep 6, 2023

Change-type: minor

Screenshot 2023-11-29 at 17 28 33 Screenshot 2023-11-29 at 17 28 50 Screenshot 2023-11-29 at 17 29 19 Screenshot 2023-11-29 at 17 28 22

@JSReds JSReds self-assigned this Sep 6, 2023
@JSReds JSReds force-pushed the add-download-image-modal branch 5 times, most recently from 29caf00 to 81f19c0 Compare September 7, 2023 11:35
@drskullster drskullster force-pushed the add-download-image-modal branch from c27f7e7 to f584d08 Compare September 15, 2023 12:29
@JSReds JSReds force-pushed the add-download-image-modal branch from f584d08 to e6ece62 Compare November 16, 2023 13:49
@JSReds
Copy link
Contributor Author

JSReds commented Nov 16, 2023

sorry for removing the contribution @drskullster 🙏 🙏 🙏 🙏

@drskullster drskullster force-pushed the add-download-image-modal branch from e6ece62 to 7f47343 Compare November 22, 2023 15:13
@JSReds JSReds force-pushed the add-download-image-modal branch 2 times, most recently from 52296c7 to 29ed598 Compare November 29, 2023 16:33
@JSReds JSReds force-pushed the add-download-image-modal branch 3 times, most recently from 8df11ce to d272696 Compare December 12, 2023 16:35
@JSReds JSReds force-pushed the add-download-image-modal branch from d272696 to b41b00c Compare December 12, 2023 17:23
@@ -32,7 +32,9 @@ const preview: Preview = {
decorators: [
(Story) => (
<ThemeProvider theme={theme}>
<Story />
<ScopedCssBaseline>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure, I believe @drskullster mentioned he was going to add this in a standalone PR. Was using this agreed upon?

);

export const transformVersions = (versions: OsVersion[]) => {
// Get a single object per stripped version, with both variants of it included (if they exist). It expects a sorted `
Copy link
Contributor

Choose a reason for hiding this comment

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

It expects a sorted `

Perhaps I am just not understanding, is this a complete thought? 😓

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch, that was an old comment, not sure where it come from, btw it was probably a sorted OsVersion array. I'll remove it as it is not really necessary.

@@ -0,0 +1,139 @@
export interface Dictionary<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, I don't want to complicate this PR further so we can leave it as is regardless of your answer:

Did we just bring these typings over from rendition or did you go through them all and filter out anything we no longer need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have removed all unnecessary types. Next step would be to unify what is used in multiple places

>
<Box display="flex" flexDirection="column">
<FormControlLabel
sx={{ opacity: isDisabled ? 0.6 : 1 }}
Copy link
Contributor

@myarmolinsky myarmolinsky Dec 12, 2023

Choose a reason for hiding this comment

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

Is 0.6 the opacity we use everywhere else? Asking because I've seen us using 0.4 in other places (but it could easily have been for a different case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should make a generic "disabled" rule in the theme adding it in design-tokens first. For now I've switched to 0.4 which is probably better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree

@JSReds JSReds force-pushed the add-download-image-modal branch 2 times, most recently from a643b34 to bccb3cd Compare December 13, 2023 11:47
@JSReds JSReds force-pushed the add-download-image-modal branch from 8e506f9 to 3bc7c0c Compare December 13, 2023 13:44
@JSReds JSReds merged commit f1fbb83 into master Dec 13, 2023
47 checks passed
@JSReds JSReds deleted the add-download-image-modal branch December 13, 2023 13:50
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.

3 participants