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.

Sign up for GitHub

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

Jump to bottom

[#8589] Student Profile: add a 'Upload/Edit Photo' tooltip #9021

Merged
merged 13 commits into from
Nov 27, 2018
Merged

[#8589] Student Profile: add a 'Upload/Edit Photo' tooltip #9021

merged 13 commits into from
Nov 27, 2018

Conversation

yaroslav-hnatiuk
Copy link
Contributor

@yaroslav-hnatiuk yaroslav-hnatiuk commented Aug 3, 2018 •
edited

Fixes #8589

Updated student profile tooltip for photo according to ticket description.

@yaroslav-hnatiuk
Copy link
Contributor Author

Hello! Could you help me? I found in a guide how to run a single test. But I want to run a single test in debugging. How can I do it? How can I run project or single test in debuggin resume? I ask because I have failed unit tests during CI build.

@sukanta-27
Copy link
Member

@YaroslavGnatyuk As you have made UI changes, you will have to run GodMode on the tests that are failing due to your changes.

@yaroslav-hnatiuk
Copy link
Contributor Author

yaroslav-hnatiuk commented Aug 4, 2018 •
edited

@sukanta-27 Thanks , do I have possibility to run tests in debug mode?

@sukanta-27
Copy link
Member

sukanta-27 commented Aug 4, 2018 •
edited

@YaroslavGnatyuk Yes.

But don't run the tests on debug mode when you are in GodMode. Read the document linked in my previous comment first before running godmode. And check afterwards to be sure that no unrelated changes have been made.

@yaroslav-hnatiuk
Copy link
Contributor Author

@sukanta-27 Could you please review my solution?

@sukanta-27
Copy link
Member

@YaroslavGnatyuk Can add a screenshot first? (As you are doing a UI change, it will help if you add a screenshot of your fix so that we know it is working)

@yaroslav-hnatiuk
Copy link
Contributor Author

@sukanta-27
screenshot from 2018-08-06 07-23-50

sukanta-27
sukanta-27 requested changes Aug 6, 2018
View reviewed changes
Copy link
Member

@sukanta-27 sukanta-27 left a comment

Choose a reason for hiding this comment

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

I guess it makes more sense to add a tooltip for Upload/Edit Photo button instead of adding it to the profile pic?

src/main/java/teammates/common/util/Const.java Outdated
@@ -433,7 +433,7 @@ private Const() {
public static final String FEEDBACK_QUESTION_MCQ_ASSIGN_WEIGHTS =
"Assign weights to the choices for calculating statistics.";

public static final String STUDENT_PROFILE_PICTURE = "Upload a profile picture";
public static final String STUDENT_PROFILE_PICTURE = "Upload a profile picture. Max size: 5MB .png .jpg";
Copy link
Member

Choose a reason for hiding this comment

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

Can change the message to something like:

Upload a profile picture (.jpg or .png, max size 5 MB)

@yaroslav-hnatiuk
Copy link
Contributor Author

yaroslav-hnatiuk commented Aug 6, 2018 •
edited

@sukanta-27 According to issue as I understand tooltip belong to profile pic. But if you think that makes more sense to change tooltip for button I can update my fix. How do you think?

@sukanta-27
Copy link
Member

@YaroslavGnatyuk From what I understood, the issue is about adding a tooltip for Update/Edit Photo, and generally it makes more sense to add a tooltip to the button rather than the profile picture (As the button is executing the action).

@yaroslav-hnatiuk
Copy link
Contributor Author

yaroslav-hnatiuk commented Aug 6, 2018 •
edited

@sukanta-27 Ok, thank you. I'm going to update the fix today.

@yaroslav-hnatiuk
Updated text and position for tooltip
ea4b347 
Tooltip appears when cursor on the "Upload/Edit Photo" button on the "Student Profile" page.
@yaroslav-hnatiuk
Merge branch 'master' into 8589-update-student-profile-tooltip
4b8e3e4
@yaroslav-hnatiuk
Copy link
Contributor Author

@sukanta-27 Hello! I have already updated fix. Could you please review my solution?
Also, screenshot attached:
screenshot from 2018-08-12 15-47-54

sukanta-27
sukanta-27 reviewed Aug 12, 2018
View reviewed changes
Copy link
Member

@sukanta-27 sukanta-27 left a comment

Choose a reason for hiding this comment

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

Just a nit. Should be okay after it.

src/main/webapp/WEB-INF/tags/student/profile/studentProfileDiv.tag Outdated
<img id="profilePic"
src="${profile.pictureUrl}"
class="profile-pic"
data-toggle="modal"
data-target="#studentPhotoUploader"
data-edit="${profile.editingPhoto}">
</div>
<div class="">
<div class="col-xs-6 col-sm-5 col-md-3 cursor-pointer"
Copy link
Member

Choose a reason for hiding this comment

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

I think the cursor-pointer here is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thaks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sukanta-27 Done, but the latest build was failed. Something went wrong with - "nvm install node" failed and exited with 3 during.

@yaroslav-hnatiuk
Copy link
Contributor Author

yaroslav-hnatiuk commented Aug 13, 2018 •
edited

@sukanta-27 CI build finished without errors. What I should to do next?

sukanta-27
sukanta-27 approved these changes Aug 13, 2018
View reviewed changes
Copy link
Member

@sukanta-27 sukanta-27 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you for your contribution.
Before this can get merged, you will have to wait for a final review from @TEAMMATES/seniordevs

@yaroslav-hnatiuk
Copy link
Contributor Author

So can I take another issue not for beginners? For example some unit tests?

@sukanta-27
Copy link
Member

So can I take another issue not for beginners? For example some unit tests?

Go ahead. 👍

@yaroslav-hnatiuk
Copy link
Contributor Author

Ok, thanks!

@wkurniawan07 wkurniawan07 changed the title Student Profile: add a 'Upload/Edit Photo' tooltip #8589 [#8589] Student Profile: add a 'Upload/Edit Photo' tooltip Aug 27, 2018
@sukanta-27 sukanta-27 added the s.ToReview The PR is waiting for review(s) label Aug 28, 2018
bqnguyen94
bqnguyen94 reviewed Aug 29, 2018
View reviewed changes
Copy link
Contributor

@bqnguyen94 bqnguyen94 left a comment

Choose a reason for hiding this comment

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

Looks like the tooltip can be added to the upload modal as well (as done here). What do you think?

wkurniawan07
wkurniawan07 approved these changes Nov 27, 2018
View reviewed changes
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

Good to go, really sorry for the long delay!

@wkurniawan07 wkurniawan07 merged commit 2aa0205 into TEAMMATES:master Nov 27, 2018
@wkurniawan07 wkurniawan07 added this to the V6.16.0 milestone Jan 11, 2019
@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.ToReview The PR is waiting for review(s) labels Jan 11, 2019
@xpdavid xpdavid added the c.Feature User-facing feature; can be new feature or enhancement to existing feature label Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bqnguyen94 bqnguyen94 bqnguyen94 left review comments

@wkurniawan07 wkurniawan07 wkurniawan07 approved these changes

@sukanta-27 sukanta-27 sukanta-27 approved these changes

Assignees

@wkurniawan07 wkurniawan07

@sukanta-27 sukanta-27

Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V6.16.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yaroslav-hnatiuk @sukanta-27 @wkurniawan07 @bqnguyen94 @xpdavid