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

refactor(parameter-pane): implement the new parameter pane #82

Merged
merged 8 commits into from
Sep 17, 2023

Conversation

kyubxy
Copy link
Collaborator

@kyubxy kyubxy commented Sep 7, 2023

decided to go with a slightly different design for categories

image

original design docs for reference

Currently, this PR doesn't animate hiding the parameter pane, though I don't think that should be a priority at this point.

With the code in this PR, we can introduce new parameter categories as separate function components. We can then add those components to the array in getAllCategories and specify whether it is an easy or expert mode option with an enum bitmask.

When reviewing, please check the way I'm selectively rendering different categories, namely the filterOnDifficulty and getAllCategories functions. I'm not sure if my method is best practice.

closes #41

@Lutra-Fs Lutra-Fs changed the title Implemented parameter pane into website refactor(parameter-pane): implement the new parameter pane Sep 7, 2023
@Lutra-Fs
Copy link
Member

Lutra-Fs commented Sep 9, 2023

image

Copy link
Member

@Lutra-Fs Lutra-Fs left a comment

Choose a reason for hiding this comment

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

The style is broken. cc: @LorenzoValentine
image

@LorenzoValentine
Copy link
Member

The style is broken. cc: @LorenzoValentine image

What do you mean by 'the style'? Do you mean that when reducing the horizontal distance of the browser, the simulation area will block the parameter pane?

Comment on lines 69 to 81
function ShowHideButton(props: {
isVisible: boolean,
setVisible: (inp:boolean)=>void,
}) {
const isVisible = props.isVisible
const setVisible = props.setVisible

return(
<BackButton onClick={() => setVisible(!isVisible)}>
{isVisible ? (<DoubleLeftOutlined />) : (<DoubleRightOutlined />)}
</BackButton>
)
}
Copy link
Member

Choose a reason for hiding this comment

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

Another issue is that, when you click the 'ShowHide' button, when the par bar is retracted, the position of this button does not correspond to the position of the par bar when it is displayed. Normally, they should be at the same level.
Hide:
Screenshot 2023-09-09 at 18 33 37

Display:
Screenshot 2023-09-09 at 18 33 32

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

buttons should now be roughly the same height now

@LorenzoValentine
Copy link
Member

Should the font size be smaller than the normal font size?
Screenshot 2023-09-09 at 18 44 15

@kyubxy
Copy link
Collaborator Author

kyubxy commented Sep 10, 2023

Should the font size be smaller than the normal font size? Screenshot 2023-09-09 at 18 44 15

image

other services like discord seem to use a similar font size for their labels and tooltips. personally, i'm okay with the size of the tooltips, making them smaller could reduce readability. thoughts? @LorenzoValentine

@kyubxy
Copy link
Collaborator Author

kyubxy commented Sep 10, 2023

new changes remove the component list idea and just conditionally renders each category. should hopefully fix any issues with unique key prop. also addressed the differing y positions of the expand button.

@Lutra-Fs
Copy link
Member

@peipacut, as you are now working on the parameter pane, could you run formatting in src/ParameterComponents and src\components\ParametersBar.tsx? thanks

@LorenzoValentine
Copy link
Member

Should the font size be smaller than the normal font size? Screenshot 2023-09-09 at 18 44 15

image

other services like discord seem to use a similar font size for their labels and tooltips. personally, i'm okay with the size of the tooltips, making them smaller could reduce readability. thoughts? @LorenzoValentine

But from your screenshot, the original text in Discord has a bold effect, this is different from the tooltips.

@kyubxy
Copy link
Collaborator Author

kyubxy commented Sep 15, 2023

latest push should resolve formatting issues.

as for the tooltips, they weren't initially bold on my end but hopefully the extra styling I added should force the tooltip text to always be unbold.

@Lutra-Fs
Copy link
Member

latest push should resolve formatting issues.

as for the tooltips, they weren't initially bold on my end but hopefully the extra styling I added should force the tooltip text to always be unbold.

I believe his meaning is that the original text should be different with tooltip's (in format)

@kyubxy
Copy link
Collaborator Author

kyubxy commented Sep 15, 2023

image

bolded the label text and left the tooltip text

Lutra-Fs
Lutra-Fs previously approved these changes Sep 16, 2023
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

ESLint found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@Lutra-Fs Lutra-Fs merged commit 7294bca into main Sep 17, 2023
3 of 4 checks passed
@Lutra-Fs Lutra-Fs deleted the implemented-parameter-pane branch September 17, 2023 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement parameter pane
3 participants