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

Different primary colour for pre-release versions #4810

Merged
merged 26 commits into from
Jul 31, 2022
Merged

Different primary colour for pre-release versions #4810

merged 26 commits into from
Jul 31, 2022

Conversation

Samq64
Copy link
Member

@Samq64 Samq64 commented Jul 14, 2022 •
edited
Loading

Resolves #4805

Changes

Adds -pre to the popup version (there wasn't enough space for the full version name) and automatically turns the logo and primary color blue on pre-release versions. The logo shown of the extensions page has to be manually updated.

Reason for changes

It makes it easier to identify pre-release versions of the extension.

Tests

Tested on Chromium 103.

Screenshot from 2022-07-20 08-06-26

@RedGuy12
Copy link
Contributor

You could set the icon in manifest.json to always be blue, then in .github/workflows/gen-manifest.js change it to the regular one

Also maybe export-ignore the blue logos? Would that only affect the release version or would it also affect the beta?

@mxmou
Copy link
Member

mxmou commented Jul 15, 2022

--blue (#175ef8) is slightly brighter and is meant to be used as a background color. Could that color be used instead?

@Samq64
Copy link
Member Author

Samq64 commented Jul 15, 2022 •
edited
Loading

--blue (#175ef8) is slightly brighter and is meant to be used as a background color. Could that color be used instead?

Sure. I didn't realize how dark it was until I used it for the header.

mxmou
mxmou reviewed Jul 16, 2022
View reviewed changes
webpages/popup/index.js Outdated Show resolved Hide resolved
webpages/settings/index.js Outdated Show resolved Hide resolved
@Samq64 Samq64 marked this pull request as ready for review July 16, 2022 12:56
@Samq64
Copy link
Member Author

Samq64 commented Jul 16, 2022

I wanted to make it clear it's a pre-release version on the popup, but it doesn't fit on one line. Should I put the version under the title?

@mxmou
Copy link
Member

mxmou commented Jul 16, 2022

I wanted to make it clear it's a pre-release version on the popup, but it doesn't fit on one line. Should I put the version under the title?

Maybe -pre could be appended to the version number?

@lisa-wolfgang
Copy link
Member

If feasible, we should change the "brand orange" used throughout the extension as well. (This would probably require a bit of CSS modification to reduce headache on version bump.)

@Samq64
Copy link
Member Author

Samq64 commented Jul 18, 2022

If feasible, we should change the "brand orange" used throughout the extension as well. (This would probably require a bit of CSS modification to reduce headache on version bump.)

OK. Should I reverse the blue and orange, or just change the orange?

@lisa-wolfgang
Copy link
Member

Changing everything to the blue would be best.

@Samq64
Copy link
Member Author

Samq64 commented Jul 18, 2022

@lisa-wolfgang Done. I also made a transparent logo so I could remove the messy logo swapping code.

@RedGuy12
Copy link
Contributor

The favicon is still orange, but it doesn't even seem to be located in this repo.

We use link[rel="icon"] instead of favicon.ico

Also it's manifest.browser_action but with that fix, I tested and it works

@Samq64
fix again
ba2834b
@Samq64
remove favicon
517b9a1 
Defaults to the extension icon instead,  is fine.
@Samq64
Revert "turn --blue orange"
fd1584c 
This reverts commit 328498a.
@Samq64
Copy link
Member Author

Samq64 commented Jul 19, 2022 •
edited
Loading

I found a race condition, but I don't run into it very often.

@RedGuy12
Copy link
Contributor

What race condition?

RedGuy12
RedGuy12 approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@RedGuy12 RedGuy12 left a comment

Choose a reason for hiding this comment

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

Lgtm

@Samq64
Copy link
Member Author

Samq64 commented Jul 20, 2022 •
edited
Loading

What race condition?

About 1/4 of the time after reloading the extension, the primary colour isn't changed. Re-opening the popup corrects it.

@Secret-chest
Copy link
Contributor

How does it look now?

@Samq64
Copy link
Member Author

Samq64 commented Jul 20, 2022

How does it look now?

Screenshot from 2022-07-20 08-06-26

compressed_Screenshot from 2022-07-20 08-09-30

@Secret-chest
Copy link
Contributor

Wow, I still think blue should be orange too

@Samq64
Copy link
Member Author

Samq64 commented Jul 20, 2022

Wow, I still think blue should be orange too

I tried that, but reverted it because it turned things like links orange, and I didn't want to make another css variable, but I can do that if you want.

@Secret-chest
Copy link
Contributor

Wow, I still think blue should be orange too

I tried that, but reverted it because it turned things like links orange, and I didn't want to make another css variable, but I can do that if you want.

Links aren't the same blue.

@Hans5958
Copy link
Member

A little bit offtopic: I see that you change it so the logo is transparent. This would make my idea of making the extension logo to be larger (as large as I did on the website) to be easier. I would follow it up on the next PR after this gets merged.

@Hans5958
Copy link
Member

Also, for the others, can we start doing a shorter -pre suffix on the manifest instead of the longer -prerelease suffix next time?

@mxmou
Copy link
Member

mxmou commented Jul 21, 2022

Wow, I still think blue should be orange too

I tried that, but reverted it because it turned things like links orange, and I didn't want to make another css variable, but I can do that if you want.

Links aren't the same blue.

In light mode they are.

A little bit offtopic: I see that you change it so the logo is transparent. This would make my idea of making the extension logo to be larger (as large as I did on the website) to be easier. I would follow it up on the next PR after this gets merged.

I don't think the logo should be much larger than the "Scratch Addons" title.

@WorldLanguages
Copy link
Member

Also, for the others, can we start doing a shorter -pre suffix on the manifest instead of the longer -prerelease suffix next time?

-prerelease is valid semver

@Hans5958
Copy link
Member

And so -pre. I wish it is shorter.

@apple502j apple502j added priority: 4 Low priority. Includes minor bugs and less important features scope: design Related to design of the extension scope: webpages Related to the web pages (settings page, pop-up, etc) scope: core Related to the core script/extension workings labels Jul 27, 2022
@apple502j apple502j merged commit 34dc37d into ScratchAddons:master Jul 31, 2022
@Samq64 Samq64 deleted the beta-icon branch July 31, 2022 19:26
apple502j pushed a commit that referenced this pull request Aug 6, 2022
@Samq64
Fix a couple bugs from #4810 (#4934)
1411f6c 
* Update global-theme.js

* Format code

* Alternative solution

* oops

* Fix icon preload path

Co-authored-by: Samq64 <Samq64@users.noreply.github.com>
@mxmou mxmou mentioned this pull request Aug 14, 2022
Open
@Samq64 Samq64 mentioned this pull request Sep 13, 2022
Closed
This was referenced May 25, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxmou mxmou mxmou left review comments

@Hans5958 Hans5958 Hans5958 approved these changes

@RedGuy12 RedGuy12 RedGuy12 approved these changes

@WorldLanguages WorldLanguages Awaiting requested review from WorldLanguages WorldLanguages is a code owner

Assignees
No one assigned
Labels
priority: 4 Low priority. Includes minor bugs and less important features scope: core Related to the core script/extension workings scope: design Related to design of the extension scope: webpages Related to the web pages (settings page, pop-up, etc)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change Scratch Addons logo for pre-release versions
8 participants
@Samq64 @RedGuy12 @mxmou @lisa-wolfgang @Secret-chest @Hans5958 @WorldLanguages @apple502j