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

course_membership.js: replace with webcomponent #4083

Merged
merged 7 commits into from
Nov 21, 2022

Conversation

freyavs
Copy link
Contributor

@freyavs freyavs commented Oct 17, 2022

This pull request removes course_membership.js and replaces the functionality with web components (using the already existing d-datalist-input), with the goal of removing jquery and converting to typescript.

The view has slightly changed from this:
2022-11-16_11-17

To this:
2022-11-16_10-22

It is progress on #3590

@freyavs freyavs changed the title course_membership.js: remove jquery and use typescript course_membership.js: remove jquery and convert to typescript Nov 3, 2022
@chvp chvp added the chore Repository/build/dependency maintenance label Nov 11, 2022
@freyavs freyavs changed the title course_membership.js: remove jquery and convert to typescript course_membership.js: replace with webcomponent Nov 16, 2022
@freyavs freyavs marked this pull request as ready for review November 16, 2022 10:22
@freyavs freyavs requested a review from a team as a code owner November 16, 2022 10:22
@freyavs freyavs requested review from niknetniko and jorg-vr and removed request for a team November 16, 2022 10:22
@jorg-vr jorg-vr added the deploy mestra Request a deployment on mestra label Nov 16, 2022
@jorg-vr jorg-vr temporarily deployed to mestra November 16, 2022 10:37 Inactive
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Nov 16, 2022
Copy link
Contributor

@jorg-vr jorg-vr left a comment

Choose a reason for hiding this comment

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

This looks good :)

I left some minor remarks but these are mostly about comments

One visual issue I noticed, could you add a bit of margin to the top or bottom of the labels? they don't look great when there is more than one row
image

Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

The code looks good, but I have a minor UX remark. If I'm correct, you can type something that is not present in the list and add it as well, but the UI isn't super clear on that because of the double focus on "search" both in the placeholder text and the info below.

Maybe searching doesn't even have to be mentioned because it's not needed?

@freyavs
Copy link
Contributor Author

freyavs commented Nov 21, 2022

The code looks good, but I have a minor UX remark. If I'm correct, you can type something that is not present in the list and add it as well, but the UI isn't super clear on that because of the double focus on "search" both in the placeholder text and the info below.

Maybe searching doesn't even have to be mentioned because it's not needed?

I agree. Would this be better, or still not clear enough?
info: "Add an existing userlabel or create a new one"
placeholder: just something like "start typing..."

@bmesuere
Copy link
Member

The code looks good, but I have a minor UX remark. If I'm correct, you can type something that is not present in the list and add it as well, but the UI isn't super clear on that because of the double focus on "search" both in the placeholder text and the info below.
Maybe searching doesn't even have to be mentioned because it's not needed?

I agree. Would this be better, or still not clear enough? info: "Add an existing userlabel or create a new one" placeholder: just something like "start typing..."

I agree with the info text, but would just use "Label" as placeholder text. This best matches the material design spec.

@jorg-vr jorg-vr merged commit 0777483 into develop Nov 21, 2022
@jorg-vr jorg-vr deleted the chore/coursemembership-jquery-removal branch November 21, 2022 16:54
@freyavs freyavs mentioned this pull request Nov 23, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Repository/build/dependency maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants