chiyadev / genshin-schedule

🕑 Genshin farming scheduler
https://genshin.chiya.dev
MIT License
72 stars 17 forks source link

Show weekly character talent materials in domain list in addition to talent materials #36

Closed luaneko closed 3 years ago

luaneko commented 3 years ago

I don't understand why the lack of such an obvious feature didn't prompt me until now.

lauslim12 commented 3 years ago

Did you mean 'weekly bosses talent materials'? If so, I think I found the solution for it.

I took the Dvalin's Plume image from the wiki.

Here's an example with Jean: image image

Here's an example with Fischl: image

Technically, I made it so the weekly bosses materials are taken from the second element of the talentMaterials array in the characters.ts file, as we know that one character only needs one weekly ascension material. I also made it so that Trounce domains could be done AnyDay. If we want to be perfectionist, we can add the Crown of Sagehood too haha!

We also have to modify the domainDropSets.ts and talentMaterials.ts to add each of the weekly bosses material to each character.

What do you think of this? (I know about the double checkbox thingy, will probably have to do a workaround of it, maybe by using two checkboxes passing different props to the Toggle.tsx component?)

luaneko commented 3 years ago

I made it so the weekly bosses materials are taken from the second element of the talentMaterials array in the characters.ts file

I think it's better to go with a separate field like talentMaterialWeekly because the main character actually requires different talent materials at different levels (see #11).

If we want to be perfectionist, we can add the Crown of Sagehood too haha!

Crown isn't farmable from domains so unfortunately it doesn't belong in the Domain List.

I know about the double checkbox thingy, will probably have to do a workaround of it, maybe by using two checkboxes passing different props to the Toggle.tsx component?

I'm not too worried about code duplication at this stage.


Otherwise I think it's great! If you have the code for this already, I think you should make a draft PR and we can work together from there.

lauslim12 commented 3 years ago

I think it's better to go with a separate field like talentMaterialWeekly because the main character actually requires different talent materials at different levels (see #11).

Looking at the wiki, I believe the Traveler only needs a Dvalin's Sigh for weekly talent materials. But sure! I also think that it is better to split them up for modularity, and because we never know when the developers might change the weekly materials (imagine requiring multiple weekly materials for a single talent level up 🤔).

Otherwise I think it's great! If you have the code for this already, I think you should make a draft PR and we can work together from there.

Alright! Let me adjust the code to your request for a bit before I make the draft PR!

EDIT: Before I forget, I think it's also good if we add the elite bosses for the ascension materials (hoarfrost cores, basalt pillars, everflame seeds, etcetera). What do you think? I have no idea where to place them in the domains, though.

luaneko commented 3 years ago

I believe the Traveler only needs a Dvalin's Sigh for weekly talent materials

I'm talking about this row on the Traveler page, which shows that different talent materials are required at different levels

image

in contrast to other characters e.g. Zhongli who only requires one type.

image

The array for talentMaterials is to be used to eventually implement #11, which is why you can't designate the second array element for the weekly material.

I think it's also good if we add the elite bosses for the ascension materials (hoarfrost cores, basalt pillars, everflame seeds, etcetera). What do you think? I have no idea where to place them in the domains, though.

To be honest, I was against this idea in the beginning because I wanted the page to be strictly focused on "domains" only, but I'm convinced that it would be much more useful to include those items as well. It should probably come as a separate PR though.

lauslim12 commented 3 years ago

Okay, I understand what you're talking about. No worries there.

To be honest, I was against this idea in the beginning because I wanted the page to be strictly focused on "domains" only, but I'm convinced that it would be much more useful to include those items as well. It should probably come as a separate PR though.

Sure! If I have any idea about it, I'll make an issue and a PR too. It would have to be after this PR, though 😅.

By the way, the draft PR is already set up and ready to be worked on together!