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

modifying guppy css for guppy migration to package.json #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Shivkant-Chauhan
Copy link

@Shivkant-Chauhan Shivkant-Chauhan commented Aug 5, 2023

There need to be some minor CSS fixes for some icons that aren't displayed after migrating guppy to package.json. help icon was not displayed due to some conflicts with webpack, so accordingly CSS needs to be fixed. This PR must be merged after fixing other configs for this package in PR#18742, so that the package works fine

Screencast.from.05-08-23.06.47.46.PM.IST.webm

@JayVivarekar
Copy link
Member

@shivkant5 Why haven't any reviewers been assigned for this PR?

@Shivkant-Chauhan
Copy link
Author

@shivkant5 Why haven't any reviewers been assigned for this PR?

I dont know why there are no reviewers on this PR (might be because this is the forked repo of guppy package). I am also not able to assign someone on this PR.

@gp201
Copy link
Member

gp201 commented Aug 9, 2023

@seanlip PTAL

@@ -50,7 +50,7 @@
background:url('./icons/keyboard.png') center no-repeat;
}
.guppy_buttons div.help{
background:url('./icons/help.png') center no-repeat;
background:url('/dist/oppia-angular/icons/help.png') center no-repeat;
Copy link
Member

Choose a reason for hiding this comment

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

As discussed, I don't think guppy should depend on impl details of oppia. The dependency should be one-way from Oppia to Guppy.

Please find a way to make this change within Oppia instead (e.g. using a more specific CSS rule to override this one (preferred), or in the worst case, using TS to modify the CSS). If you want to modify this fork, then the changes need to be general, otherwise it will become hard to maintain and understand.

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

Successfully merging this pull request may close these issues.

4 participants