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

Chore: Exercise Overview adjustments and PR cleanup #100

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

julian-wls
Copy link
Contributor

@julian-wls julian-wls commented Nov 9, 2024

Description

This PR is follow-up PR to #42 in order to resolve some minor problems, that were pointed out here. Logic has been extracted into view models and strings have been replaced with resource strings. Furthermore, it introduces modifications to the exercise overview and fixes design issues (see screenshots).
Merging this branch will solve #75

Screenshots

Before:

ScreenshotBefore

After:

ScreenshotAfter

@julian-wls julian-wls self-assigned this Nov 9, 2024
@julian-wls julian-wls linked an issue Nov 9, 2024 that may be closed by this pull request
@julian-wls julian-wls added the ready for review This PR can be reviewed label Nov 13, 2024
@julian-wls julian-wls marked this pull request as ready for review November 13, 2024 14:57
Copy link
Collaborator

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

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

Very nice, just some minor changes :)

Copy link
Collaborator

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

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

Edit: see change requests below

Copy link
Collaborator

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

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

Sorry, I should properly test the changes before prematurely approving.

I just noticed that the exercise info container is cut off for quiz exercises:
image

@julian-wls
Copy link
Contributor Author

I just noticed that the exercise info container is cut off for quiz exercises:

Could you check again? I was not able to reproduce this issue. The changes could be resolved now, as the buttons are not displayed anymore.

Copy link
Collaborator

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

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

The issue indeed seems to be fixed by now.

However I noticed, that the Exercise titles are not displayed anymore, but it seems that this is an issue on develop aswell. If you know how this could be fixed quickly we could include it in this PR, otherwise let's create a separate issue for this.

EDIT: I just noticed that this is a bug on the develop branch, I'll create an issue for it.

grafik

Copy link
Contributor

@TimOrtel TimOrtel left a comment

Choose a reason for hiding this comment

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

Overall, looks very good. Thank you for cleaning up the hardcoded texts.

@julian-wls julian-wls added ready to merge This PR can be merged and removed ready for review This PR can be reviewed labels Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge This PR can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Communication: Clean up PR #42
3 participants