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

Fix #20166 : Updates character ‘s’ Next to Edit Icon in ‘Insert Image’ of RTE with 'SVG' #20209

Merged
merged 4 commits into from Apr 29, 2024

Conversation

Akhilesh-max
Copy link
Contributor

@Akhilesh-max Akhilesh-max commented Apr 24, 2024

Overview

  1. This PR fixes [BUG]: Unexpected Character ‘s’ Beside Edit Icon in insert Image of the RTE on Contributor Dashboard #20166.
  2. This PR does the following: it updates character ‘s’ Next to Edit Icon in ‘Insert Image’ of RTE with 'SVG' as the edit icon opens the SVG editor.

Essential Checklist

Please follow the instructions for making a code change.

  • I have linked the issue that this PR fixes in the "Development" section of the sidebar.
  • I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
  • I have written tests for my code.
  • The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
  • I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

Before :
Screenshot 2024-04-24 at 11 01 26 PM

After :
Screenshot 2024-04-25 at 12 47 10 PM

PR Pointers

@Akhilesh-max Akhilesh-max requested a review from a team as a code owner April 24, 2024 17:32
Copy link

oppiabot bot commented Apr 24, 2024

Assigning @kevintab95 for the first pass review of this PR. Thanks!

@seanlip
Copy link
Member

seanlip commented Apr 24, 2024

@Akhilesh-max Thanks for the fix. The pencil should take priority and the "SVG" is just an annotation to it. Can you make the "SVG" text smaller, or the pencil icon bigger, or put the "SVG" text under the pencil icon or somewhere else so that it looks cleaner/clearer?

Thanks.

@seanlip seanlip assigned Akhilesh-max and unassigned kevintab95 Apr 24, 2024
@Akhilesh-max
Copy link
Contributor Author

Akhilesh-max commented Apr 24, 2024

@seanlip Looks good? Updated the image in the PR.

@seanlip
Copy link
Member

seanlip commented Apr 24, 2024

@Akhilesh-max Better, but still one question. Why is the "SVG" font a serif font? It looks out of place with the rest of the text on the screen.

@Akhilesh-max
Copy link
Contributor Author

Akhilesh-max commented Apr 25, 2024

@seanlip, the different font was being inherited from the i tag. I had two options to address this: either change the font family of the SVG text to match with the rest of the page or move it outside the i tag and adjust the alignment accordingly. I decided to go with the first option and changed the font family of the SVG text. However, please let me if it's ok this way. (Have updated the image in the description)

Thanks.

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@seanlip
Copy link
Member

seanlip commented Apr 25, 2024

Assigning @kevintab95 for codeowners.

@seanlip seanlip enabled auto-merge April 25, 2024 07:49
Copy link
Member

@kevintab95 kevintab95 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Akhilesh-max!

@seanlip seanlip added this pull request to the merge queue Apr 29, 2024
@kevintab95 kevintab95 removed their assignment Apr 29, 2024
@oppiabot oppiabot bot added the PR: LGTM label Apr 29, 2024
Copy link

oppiabot bot commented Apr 29, 2024

Hi @Akhilesh-max, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks!

Merged via the queue into oppia:develop with commit 00300a9 Apr 29, 2024
80 checks passed
@Akhilesh-max Akhilesh-max deleted the SVG-Editor branch April 29, 2024 07:57
shivanandan17 pushed a commit to shivanandan17/oppia that referenced this pull request Apr 29, 2024
…Image’ of RTE with 'SVG' (oppia#20209)

* updates

* updates

* updates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: Unexpected Character ‘s’ Beside Edit Icon in insert Image of the RTE on Contributor Dashboard
3 participants