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

Issue/settings dialog #1540

Merged
merged 17 commits into from
Jan 30, 2024
Merged

Conversation

cayb0rg
Copy link
Contributor

@cayb0rg cayb0rg commented Oct 31, 2023

Fixes #1512

  • Hides embedded only option for students and student made widgets and increases dialog's max-height

Fixes #1504

  • Updates settings dialog view and edit permissions for grandfathered-in users

Additional

  • Added error message for when you try to remove guest access from a student-made widget in instance manager
  • Adds 'owner' tag to collaborator dialog

Continuation of #1516

Copy link
Member

@clpetersonucf clpetersonucf 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 left one request below based on displaying scores, but I'd also like to see some level of responsiveness added to the settings dialog. Currently, if a user is interacting with the dialog with a lower resolution display, the bottom of the dialog is cut off and prevents them from selecting the Save and Cancel options at the bottom.

src/components/my-widgets-scores.jsx Outdated Show resolved Hide resolved
@cayb0rg
Copy link
Contributor Author

cayb0rg commented Jan 11, 2024

I overlooked the fact that students should still be able to view guest scores, thank you! I added an is_student check to play_logs_get to exclude information such as user ID, first name, last name, and username from the results. The front-end will combine these into a single "Guests" score. It will still display the number of students.

I've also updated the setting dialog to be more responsive. Still working on the attempts for mobile.

Copy link
Member

@clpetersonucf clpetersonucf left a comment

Choose a reason for hiding this comment

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

Some small things here and there that caught my eye.

Additionally, there appear to be some issues with rendering the settings dialog related to the responsive styles you added, at least on my end:

Screenshot 2024-01-23 at 2 44 25 PM

fuel/app/classes/materia/widget/question.php Outdated Show resolved Hide resolved
src/components/my-widgets-settings-dialog.jsx Outdated Show resolved Hide resolved
src/components/my-widgets-scores.jsx Outdated Show resolved Hide resolved
src/components/my-widgets-scores.scss Outdated Show resolved Hide resolved
@cayb0rg
Copy link
Contributor Author

cayb0rg commented Jan 25, 2024

Thanks for spotting these!

Additionally, there appear to be some issues with rendering the settings dialog related to the responsive styles you added, at least on my end:

Screenshot 2024-01-23 at 2 44 25 PM

I couldn't replicate this issue (which I assume is the date picker wrapping even though there is room for it). Could you possibly specify the browser, screen width, and/or zoom level?

@clpetersonucf
Copy link
Member

I couldn't replicate this issue (which I assume is the date picker wrapping even though there is room for it). Could you possibly specify the browser, screen width, and/or zoom level?

Firefox, 100% zoom, 1792px. I don't recall encountering this issue previously, but it definitely appears related to the addition of flex-wrap on line 301 of the settings-dialog stylesheet.

Copy link
Member

@clpetersonucf clpetersonucf left a comment

Choose a reason for hiding this comment

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

Approved. Really great work here: your solution to score display when a user becomes a student is inspired. It's non-destructive, still gives user score access, but decontextualizes the scores until/unless the student role is replaced.

Well done, I think this is good to go.

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.

2 participants