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

Icon Updated. #31

Merged
merged 2 commits into from
Jan 2, 2025
Merged

Icon Updated. #31

merged 2 commits into from
Jan 2, 2025

Conversation

CWAbhi
Copy link
Contributor

@CWAbhi CWAbhi commented Jan 1, 2025

#30
Updated the icon.
and prepare an activity bundle (the .xo file)

@quozl
Copy link
Contributor

quozl commented Jan 1, 2025

@anya-xcode, please review. See our guide for reviewers.

@anya-xcode
Copy link

Hi @quozl ,

I’ve reviewed the pull request, and I wanted to share my thoughts:

The replacement of the icon is correct, but I think we could enhance the design further by using a more stylish or modern icon rather than the current one. A more polished icon could improve the overall UI/UX feel.

I’m unable to review the parts related to "Prepare an activity bundle" and "Demonstrate test in Sugar" at the moment, as I don’t have enough context or access to the relevant components. If you could clarify these areas or share more details, I’d be happy to take another look.

@quozl
Copy link
Contributor

quozl commented Jan 1, 2025

Thanks. I agree replacement of icon is correct.

The icon was not designed to be stylish or modern, but to be comprehended by an individual who has used a pencil and not a phone or computer. That's why it renders as a pencil cross. Changing it would require change to quite a lot of activities, where it is used in context for clearing edits. Therefore a wider consultation would be required, outside the scope of a pull request.

Yes, it looks like neither of you have used Sugar and so don't know how to prepare an activity bundle or test inside Sugar.

The pull request includes a copy of an activity bundle. While helpful to show the activity bundle was incorrectly prepared (it includes .git, and doesn't have the right filename), we don't want the activity bundle in the git repository.

Activity bundles are created using python setup.py dist_xo and the result is written to a directory dist.

Testing requires running Sugar. I've done that, and while the Clear Canvas button is correct, my test was a fail because the activity icon is misaligned, the Stop button did not work. I don't know if the pull request introduced these problems or if they were there already. It is critical to test before and after making commits, please.

p31

@CWAbhi
Copy link
Contributor Author

CWAbhi commented Jan 2, 2025

@quozl my bad i will create an activity bundle as you mention and commit changes

@CWAbhi
Copy link
Contributor Author

CWAbhi commented Jan 2, 2025

@quozl I have deleted the activity bundle from the repo.
The icons are placed correctly in my local environment and the stop button is not working in local but it was not due to my pull request it was before my pull request. I have not yet setup the sugar environment for testing .
@anya-xcode can you help us out with the testing?
PHOTO-2025-01-02-11-42-45

@anya-xcode
Copy link

anya-xcode commented Jan 2, 2025

@CWAbhi I was following the setup instructions for Sugar, but I noticed that the documentation only provides steps for Linux-based systems, and there aren't any specific instructions for macOS. As a result, I'm currently unable to complete the setup on my Mac.

@quozl quozl mentioned this pull request Jan 2, 2025
@quozl
Copy link
Contributor

quozl commented Jan 2, 2025

@CWAbhi thanks, but testing inside a web browser is not the target environment.

@quozl quozl merged commit d044cd7 into sugarlabs:master Jan 2, 2025
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.

3 participants