-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Fix #16830: Remove unclear sentence from save progress popup #16838
Conversation
Hi @JayVivarekar, @aasiffaizal -- could one of you please add the appropriate changelog label to this pull request? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we remove it from the below file as well?
Line 1125 in ab66f74
"I18N_SAVE_EXPLORATION_PROGRESS_TEXT_4": "Text displayed on the save progress menu of exploration lesson info modal telling the users that the progress URL generated will save their progress for the next 72 hours only.", |
Unassigning @aasiffaizal since the review is done. |
Hi @Kej-r03, it looks like some changes were requested on this pull request by @aasiffaizal. PTAL. Thanks! |
@Kej-r03 In your screenshot the text to copy seems to be kinda broken, can you also fix that? |
Hi @Kej-r03, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks! |
Changes look good, but the section containing the link to be copied seems broken (although it appears to be unrelated to the changes, so I'm not sure why), please take a look! |
@nithinrdy I am getting this after merging the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kej-r03 the left edge of the div containing the url is supposed to be aligned with the text, like so:
Should be a quick CSS change, could you PTAL?
Unassigning @nithinrdy since the review is done. |
Hi @Kej-r03, it looks like some changes were requested on this pull request by @nithinrdy. PTAL. Thanks! |
@nithinrdy Hopefully this solves the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@aasiffaizal PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Hi @Kej-r03, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks! |
@aasiffaizal PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Hi @aks681, this PR is ready to be merged. Author of this PR does not have permissions to merge this PR. Before you merge it, please make sure that there are no pending comments that require action from the author's end. Thanks! |
@aks681 Please merge the PR |
…pia#16838) * Remove unclear sentence from save progress popup * Remove remaining unclear text and fix broken copy link for save progress * Revert config property change * Fix copy url textbox UI * Fix URL copy CSS
Overview
Essential Checklist
Proof that changes are correct
Proof of changes on desktop with slow/throttled network
Proof of changes on mobile phone
Proof of changes in Arabic language
PR Pointers