Skip to content

Fixed Upload Data Modal and Download Data Modal#331

Open
hans-hc wants to merge 5 commits intomainfrom
281-make-the-ui-more-responsive-to-different-screen-sizes
Open

Fixed Upload Data Modal and Download Data Modal#331
hans-hc wants to merge 5 commits intomainfrom
281-make-the-ui-more-responsive-to-different-screen-sizes

Conversation

@hans-hc
Copy link
Contributor

@hans-hc hans-hc commented Jan 14, 2026

as title indicates, fixed the sizing glitches when the window is squished. Also slightly adjusted the download data modal so by default full window the box isnt surpassing the height of the window.
image
image

as title indicates, fixed the sizing glitches when the window is squished. Also slightly adjusted the download data modal so by default full window the box isnt surpassing the height of the window.
Copilot AI review requested due to automatic review settings January 14, 2026 17:29
@hans-hc hans-hc linked an issue Jan 14, 2026 that may be closed by this pull request
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request addresses sizing glitches in the Upload Data Modal and Download Data Modal when the browser window is resized or on smaller screens. The changes add comprehensive responsive styles using CSS media queries at consistent breakpoints (1400px, 1200px, 1000px, 850px) to ensure modals and their contents scale appropriately. Additionally, the default height of the download modal's table wrapper has been reduced from 35rem to 30rem to prevent it from exceeding the viewport height on full-screen windows.

Changes:

  • Added responsive media queries to upload form component for adjusted sizing at smaller breakpoints
  • Added responsive padding and width adjustments to base modal component
  • Added comprehensive responsive styles to upload and download modals with progressive sizing reduction
  • Reduced default table wrapper height in download modal from 35rem to 30rem

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
UploadForm.module.scss Added responsive styles for upload form container, images, and content at 1200px, 1000px, and 850px breakpoints
BaseModal.module.scss Added responsive padding and max-width adjustments for smaller screens at 1200px and 850px breakpoints
uploadModal.module.scss Added comprehensive responsive styles for modal dimensions and spacing at 1200px, 1000px, and 850px breakpoints
downloadModal.module.scss Reduced default table height and added responsive styles at 1400px, 1200px, 1000px, and 850px breakpoints

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

width: 60rem;

.tableWrapper {
height: 32rem;
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

The table wrapper height at the 1400px breakpoint (32rem) is larger than the default height (30rem). This creates an inconsistent responsive behavior where the table actually grows when moving from full window size to 1400px breakpoint, which contradicts typical responsive design patterns where content should shrink or maintain size at smaller breakpoints.

Suggested change
height: 32rem;
height: 30rem;

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@hydrowoxy hydrowoxy left a comment

Choose a reason for hiding this comment

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

You've overlooked so much many things fool!

Extremely infinite major functional hugepick is the spacing under the upload/download button when I have it in split screen (see below)

I can't believe you would be so careless in your blumbering.... big buffoon I swear. Don't let it happen again.

Image Image

Both are half my screen but the download one is without a doubt a TON more cramped below the red button, and the upload one has wayyy too much padding below it? Bakaaaa!!!

@hydrowoxy hydrowoxy requested a review from gr812b January 14, 2026 18:27
@hans-hc
Copy link
Contributor Author

hans-hc commented Jan 14, 2026

I would never address your changes, peasant! Regrettably, you lot have forced my hand. I will comply...just this once...but it is with extreme hesitation and reluctance that I accede to your commands.

image image

Copy link
Collaborator

@gr812b gr812b left a comment

Choose a reason for hiding this comment

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

You blithering fool! You absolutely definitively must implement this in the UI folder of components and do as much as you can in there lest it cascade in a catastrophically faulty manner. More work for you <3
Looks terrible as well. This is the antithesis of the correct end product. You have created something so diametrically opposite to what we expected that you absolutely need to start worrying.
Be concerned about mobile sizes whether you want to or not. It will be seamless to use on mobile without a ground up redesign.

width: 35rem;
padding: 0 1.5rem;

.submitButton {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you put this styling directly into our button component? Might make it cascade to the whole codebase a bit better

}

@media screen and (max-width: 1200px) {
.downloadModal {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sorta stuff could also be done in baseModal maybe so itll affect this, the upload and the preset and every other modal down the line

pre approved by kai and salma for real
Some more tweaks on the home page, and made the preset card modal when clicked from the home screen responsive. Removed upload file option from preset card as per instructed
@hans-hc
Copy link
Contributor Author

hans-hc commented Jan 20, 2026

image image

Copy link
Collaborator

@gr812b gr812b left a comment

Choose a reason for hiding this comment

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

The 1000 and 850 breakpoints feel a bit odd, last I believe there are standard ones like 1400+ for ultrawide, down to 1024 for normal, then down to 768 for handhelds and <768 for rest. Might wanna just change those. Might also be good to write a mixin in the @Styles folder to centralize these values

Comment on lines +133 to +139
max-width: 400px;
}

.stepImage {
max-width: 400px;
min-height: 250px;
height: 250px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

no px!

Comment on lines +166 to +167
min-height: 250px;
height: 250px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

No px!

Comment on lines +204 to +205
min-height: 200px;
height: 200px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

No px!

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the future, before committing just check what you have made changes to, I assume this was accidental. Useful to make sure you intended to make those changes. I'll fix tn!

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 forgot to review, sorry. I will fix that asap, thank you

min-height: 16rem;
}

@media screen and (max-width: 850px) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

850 corresponds to what device?

Comment on lines -8 to -25
padding: 0 1rem;
max-width: 90rem;

.formWrapper {
display: flex;
gap: 2rem;

.uploadForm {
display: flex;
flex-direction: column;
align-items: center;
width: 50%;

.title {
all: unset;
@include fonts.title-large;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seemingly lots of changes that would change normal screen size behaviour?

Copy link
Contributor

@hydrowoxy hydrowoxy left a comment

Choose a reason for hiding this comment

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

what kai said

<FileTable
files={getFolders(files || [])}
selectedFiles={selectedFiles}
setSelectedFiles={setSelectedFiles}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like even on normal screen sizes these changes eliminate the upload functionality entirely from the preset files modal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC, I thought we agreed that the upload functionality wasnt necessary when clicking on the preset files in general?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, only on mobile sizes. Neccesary on desktop

@@ -7,45 +7,58 @@
background: var(--background);
Copy link
Contributor

@hydrowoxy hydrowoxy Jan 20, 2026

Choose a reason for hiding this comment

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

Footer looks a bit cramped on the right, Even if i elongate the window this persists

Image

gap: 1rem;
}
}
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick I suppose but between the grid having 2 columns and 1 column it looks a bit awkward around the threshold like so

Image

…different screen sizes. Added ultrawide/high resolution monitor compatibility
@hans-hc
Copy link
Contributor Author

hans-hc commented Feb 11, 2026

Wasnt entirely sure about the mixin thing so I would much appreciate a checkup on the recent commit

Copy link
Collaborator

@gr812b gr812b left a comment

Choose a reason for hiding this comment

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

Just gonna review functionality first

Image No space between button and upload box. Also did you make the default one full screen smaller? Should remain at original size for full sized screens.

At 1000px and 1024px it changes size. Why at both so close? Just stick to one (1024)

Image This one is also smaller. Keep the original size for full-sized screens. Also button spacing wrong here too. Image When shrinking the screen it can look like this, need reactivity for the table Image For the select modal, the table shrinks properly. Strange that its diff here and on download. Also as stated elsewhere need that functionality

import { showErrorToast, showSuccessToast } from '@components/ui/toastNotification/ToastNotification';
import { rightArrowIcon } from '@assets/icons';
import styles from './uploadModal.module.scss';
import baseModalStyles from '@components/ui/baseModal/BaseModal.module.scss';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't do this!

Styles are all co-located. If you have styles in baseModalStyles, they are meant to apply to parts in the baseModal. When I suggested doing styles in baseModal this was because you could just apply the styles to the root directly. You should never import a different style file

@gr812b
Copy link
Collaborator

gr812b commented Feb 19, 2026

Wasnt entirely sure about the mixin thing so I would much appreciate a checkup on the recent commit

Mixins look perfect. Just make sure to use these everywhere throughout the codebase

Copy link
Collaborator

@gr812b gr812b left a comment

Choose a reason for hiding this comment

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

Not gonna go through it all but you should use your defined variables and mixin at each of these spots

.submitButton {
margin-top: 1.25rem;
/* Responsive styles for smaller desktop windows */
@media screen and (max-width: 1400px) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use your mixin

}
}

@media screen and (max-width: 1024px) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use your mixin

}
}

@media screen and (max-width: 768px) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use your mixin

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.

Make the UI more responsive to different screen sizes

4 participants