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: cf-375 fullscreen for graphical selector #950

Merged
merged 16 commits into from
Mar 14, 2023

Conversation

ollibowers
Copy link
Collaborator

added fullscreen for graphical selector.
also split out the graphical selector file because I DIE TRYING TO EDIT THAT!!!
I DIED!

But now it pretty, and clean, and actually dont blow up eslint 🍰

I WOULD LOVE YOUR FEEDBACK ON DESIGN, I DO NOT KNOW IF IT GOOD!!!!!!!!!!!

Circles.Mozilla.Firefox.2022-12-25.00-30-59_Trim.mp4

@github-actions
Copy link

github-actions bot commented Dec 24, 2022

Test Results

142 tests   142 ✔️  2m 43s ⏱️
  14 suites      0 💤
    2 files        0

Results for commit 73bb375.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@leonardo-fan leonardo-fan left a comment

Choose a reason for hiding this comment

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

Nice job cleaning up the file structure, I like the look of fullscreen and the drawer! Left a few comments to address, would you also be able to add an index.ts file for the components you've created please (just for consistency's sake) :). Also with the SidebarDrawer, could you push it out to the src/components folder, could be reused in the future. There's also an edge case bug after switching from normal to fullscreen and vice versa then making the browser window bigger, see the video attached. Another small thing to change is that the Tabs could use a little left padding as it looks a bit cramped (i can chuck this into another ticket as it isn't really related to your feature if you want though).

Screen.Recording.2023-01-18.at.11.35.51.pm.mov

image

frontend/src/pages/GraphicalSelector/GraphicalSelector.tsx Outdated Show resolved Hide resolved
frontend/src/pages/GraphicalSelector/styles.ts Outdated Show resolved Hide resolved
frontend/src/pages/GraphicalSelector/GraphicalSelector.tsx Outdated Show resolved Hide resolved
frontend/src/pages/GraphicalSelector/CourseGraph/styles.ts Outdated Show resolved Hide resolved
@imagine-hussain
Copy link
Contributor

are there any updates on this?

@ollibowers ollibowers enabled auto-merge (squash) March 8, 2023 07:49
@ollibowers ollibowers requested a review from leonardo-fan March 9, 2023 07:43
Copy link
Contributor

@leonardo-fan leonardo-fan left a comment

Choose a reason for hiding this comment

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

lgtm! I like it picasso

@ollibowers ollibowers merged commit 0ef8189 into dev Mar 14, 2023
@ollibowers ollibowers deleted the olli/gs/cf-375-fullscreen branch March 14, 2023 13:44
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.

4 participants