-
Notifications
You must be signed in to change notification settings - Fork 270
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
ACHIEVEMENTS: Support achievements with no matching Steam achievement #1508
base: dev
Are you sure you want to change the base?
Conversation
…nt is not on Steam
The boilerplate for achievement categories was also moved to its own file.
Added a LinkOff icon and fixed coloring for AchievementEntries
Alignment now also matches Milestones page
Also minor formatting changes
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.
Overall, this looks good to me. @Snarling for approval on the idea, since this would be a significant (but IMO good) shift in how we handle achievements.
# Make background transparent and replace green with black | ||
# The icons will be recolored by css filters matching the player's theme | ||
# MacOS uses FreeBSD-style sed instead of GNU sed | ||
sed -i '' "s/fill:#000000;/fill-opacity: 0%;/g" "$i" |
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.
Instead of forking off a whole new script, use the following syntax:
sed -i"" "s/fill:#000000;/fill-opacity: 0%;/g" "$i"
The lack of space after the -i
is the key here. This works for GNU sed (I tested it); it should work for BSD (aka MacOS) sed as well (you can test).
// Most parts of the four categories in the old code were very similar (besides the content of | ||
// AccordianDetails), with the Acquired category having a few differences, | ||
// although both the Acquired and Locked categories also had an extra prop in the AccordianDetails. | ||
// The 264px minWidth feels scuffed, but fixes an edge case. |
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.
Please say what the edge case is.
sx, | ||
children, | ||
}: React.PropsWithChildren<IProps>): JSX.Element { | ||
// Most parts of the four categories in the old code were very similar (besides the content of |
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.
These lines can probably be removed. Comments should document the code as it is, not in reference to as it was. It was useful to read right now, though.
title: string; | ||
achievements: { achievement: Achievement }[]; | ||
allAchievements?: { achievement: Achievement }[]; | ||
sx?: boolean; |
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.
The sx
prop name doesn't make sense, and also it isn't the right type for a usual sx
prop.
I would either call this something like "pad", or just pass a full sx
through.
Bitburner achievements have been limited by Steam's cap of 100 achievements for games with limited profile features, but we can add more achievements internally. This change makes a few changes to allow the creation of new Bitburner achievements with no equivalent Steam achievement, including adding an optional NotInSteam boolean to the Achievement interface, and updates the Achievements page with new UI to display this to players. I tested the changes by temporarily setting NotInSteam to true for several existing achievements, and finally adding a new achievement (all in a different branch not in this pull request,
achievements-test
in my fork). The achievement README, which describes how to add a new achievement, was updated to (hopefully) explain how to do so going forward.The achievements still properly save and load on both the web and electron versions, and connecting to Steam did not cause any issues.
Screenshots of UI changes:
(note that the branch actually being merged would still show, e.g.,
Acquired (1/98, 1/98 for Steam)
)