chiyadev / genshin-schedule

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

Add Weekly Talent Materials #37

Closed lauslim12 closed 3 years ago

lauslim12 commented 3 years ago

Problem

Issue number #36 (lack of weekly boss materials).

What I Added

Results

With Diluc:

image

With Keqing:

image

With Ayaka (should not be displayed because she does not have any weeklies):

image

With Ganyu (have not added the Golden House yet, will do in a separate PR after this is done):

image

Bugs

When clicking on the Show on Schedule button, the talent level-up material will also get clicked 😅. I suggest to pass a different prop or create a new checkbox whose purpose is to add the weekly boss to the list of tasks to be done.

EDIT: After reading the codebase further, I believe we can also make use of the global state variable which is the Config object at the utils/configs.ts file. We can add a new subtype inside the Config object, let's say weeklyBosses, and it will be an array which stores the strings Dvalin, Andrius, and Childe. The strings there will be used in the Home/DomainView/index.tsx file to render the stored 'bosses'.

Then, in order to trigger the storage, the easiest way is that we have to make a new component, let's say ToggleWeekly.tsx. This component will be exactly the same as Toggle.tsx, with the difference only being that the useConfig that we use will be the weeklyBosses.

Well, actually we do not need to create a new component, just an if statement in the Toggle.tsx to differentiate it from the talent materials will suffice. In the MaterialDisplay component, we can pass a new prop, let's say isWeekly to the Toggle component. If isWeekly is true, then we will render the checkboxes for the bosses. If isWeekly is false, then we will render the checkboxes for Common Ascension Materials.

Still, in this case, making a new component is probably easier rather than passing props around.

Closing

Thank you!

Resolves #36

luaneko commented 3 years ago

I believe we can also make use of the global state variable which is the Config object at the utils/configs.ts file.

To be precise it's not a global state variable, it's passed on via a React context :) I don't think there's any global state in this codebase.

We can add a new subtype inside the Config object, let's say weeklyBosses, and it will be an array which stores the strings Dvalin, Andrius, and Childe. The strings there will be used in the Home/DomainView/index.tsx file to render the stored 'bosses'.

Instead of storing boss names, we should store the names of characters which require such bosses, so the field would be named like charactersWeekly.

In the MaterialDisplay component, we can pass a new prop, let's say isWeekly to the Toggle component. If isWeekly is true, then we will render the checkboxes for the bosses. If isWeekly is false, then we will render the checkboxes for Common Ascension Materials.

We can simplify that by simply changing this line

const [list, setList] = useConfig("characters");

like this

const [list, setList] = useConfig(isWeekly ? "charactersWeekly" : "characters");

Then there would be no need for a new component and duplicated code. The isWeekly prop would be passed to MaterialDisplay which just passes it onto Toggle.

lauslim12 commented 3 years ago

To be precise it's not a global state variable, it's passed on via a React context :) I don't think there's any global state in this codebase.

My bad! Sorry!

As for your requested changes, I have done them as requested, and I have also made it so that when we click on one checkbox, the other does not get clicked too (the bug has been resolved).

image

I have a question before rendering them on the main page though. What do you think is the best way to display the weekly domains?

Should we do it like this?

image

If we do it like this, there appears a bug again where the weekly domain does not appear if the current day is not the talent day of the character. Let me try something along the way.

EDIT 1: Nevermind, I fixed the bug already and it has rendered properly.

image

EDIT 2: We know that the character's card will be darkened a bit when we add the normal talent material to the list of tasks to be done. The question is, should I also add the darkened effect if we have added the weekly talent material to the list of tasks to be done? Right now, when we add the weekly talent material to the list of tasks, the card does not get darkened, it stays the same color, as seen in below picture.

image

Thank you!

luaneko commented 3 years ago

should I also add the darkened effect if we have added the weekly talent material to the list of tasks to be done?

Yup, let's make it dark when it's scheduled for either talent or weekly materials.

lauslim12 commented 3 years ago

All right! All done!

image

image

lauslim12 commented 3 years ago

Okay! Done as requested!

luaneko commented 3 years ago

Thanks a lot!