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

reiterate->duplex.tsx #1280

Merged
merged 5 commits into from
Jul 9, 2024
Merged

Conversation

Parkusisafk
Copy link
Contributor

No description provided.

Copy link
Collaborator

@matthias-luger matthias-luger left a comment

Choose a reason for hiding this comment

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

  1. Don't use the replace function on all enchantments, as most of the time it is not necessry. A simple check for the name should be enough (see my suggestion)
  2. Why do you check for "Ultimate_Reiterate" as well? As far as I know it should always be without underscore
  3. Put the replace above line 459. In practice it should not matter as there is always a color, but the element above would be wrong, it there wouldn't be a color present

@matthias-luger
Copy link
Collaborator

matthias-luger commented Jul 7, 2024

@Parkusisafk friendly reminder

@Parkusisafk
Copy link
Contributor Author

got it!

I checked the hypixel api and the raw output I'm getting is "Ultimate_Reiterate"
All good though the underscore should have been removed while indexing I guess

@Parkusisafk
Copy link
Contributor Author

that was my bad I didn't read the code thoroughly

@@ -456,7 +456,11 @@ function AuctionDetails(props: Props) {
{auctionDetails && auctionDetails!.enchantments.length > 0 ? (
<ul className={styles.list}>
{auctionDetails?.enchantments.map(enchantment => {
if(enchantment.name === "Ultimate Reiterate"){
enchantment.name = "Duplex";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a specific reason you just called it Duplex? We show all the Ultimate enchants with the "Ultimate" prefix to make it more obvious and easier to find (for example in the item filter search)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Parkusisafk After this is cleared, I will merge the PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, did ya'll recently changed ultimate enchants to add the prefix "Ultimate"? When I was doing the change I remember that I stumbled across a terminator with just "Soul Eater", so I assumed that the ultimate enchants did not have the "Ultimate" prefix, and instead "Ultimate Reiterate" is just some raw data carried over from the api with the "Ultimate" still on it. Just now I checked and indeed the prefix is present. Thank you for correcting me

To address the ".replace" function, at first I was unsure of the content of the string [enchantment.name], ie. there might be other characters inside (example: Enchantment_Ultimate_Reiterate), and there might be other functions such as .slice that removes the extra part and extract the enchantment. So putting ".replace" was just being cautious

@matthias-luger
Copy link
Collaborator

LGTM!

@matthias-luger matthias-luger merged commit d2229b2 into Coflnet:develop Jul 9, 2024
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.

2 participants