Skip to content

add sidebar + sort modules #2225

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

Closed
wants to merge 20 commits into from
Closed

Conversation

NOTMEE12
Copy link

@NOTMEE12 NOTMEE12 commented Jun 2, 2023

I will add sidebar (maybe also make it look better).
Also I feel that sorting by Most Usefull stuff, Advanced stuff, is not the best sorting, so I will sort modules by Usecase (pygame.display will go to Window section, etc.)

@NOTMEE12 NOTMEE12 requested a review from a team as a code owner June 2, 2023 15:15
@NOTMEE12 NOTMEE12 marked this pull request as draft June 2, 2023 15:17
@NOTMEE12
Copy link
Author

NOTMEE12 commented Jun 3, 2023

mockup of what I want to get:
image
btw. this is a screenshot edited in paint, not a index.html file. This is not done in the time that I'm writing this.

@yunline yunline added the docs label Jun 4, 2023
@NOTMEE12 NOTMEE12 closed this Jun 4, 2023
@NOTMEE12 NOTMEE12 reopened this Jun 4, 2023
@Notenlish
Copy link
Contributor

image
why are you adding '▼' into the text itself? You should add it as a seperate element because:
A) It wont appear as text when you hold down the mouse
B) Cleaner implementation

image
Secondly, you should make the width of the sidebar fixed, not based on the window width. Because it just leads to the sidebar getting narrower when you make the window narrower. Instead it should ideally have more or less a fixed width. If the window is too narrow, It should work like how the godot docs handles it.

Thirdly, I dont think that making the sidebar have a gradient for background makes sense.

Also, the sidebar shouldn't change width when hovering over it. It just makes it distracting.

@NOTMEE12
Copy link
Author

NOTMEE12 commented Jun 8, 2023

About 1:
I can make it as an new element, but I dont know if it will be on the same line.
About 2:
I'm adding support for smaller screens ( < 1100 width ). It will just create a navbar on top of the screen and if you click it, it will expand and show more options.
About 3:
It looks very good when you add shadow :).

About last:
Yeah, I can remove that.

@Notenlish
Copy link
Contributor

Notenlish commented Jun 8, 2023

About 2: I'm adding support for smaller screens ( < 1100 width ). It will just create a navbar on top of the screen and if you click it, it will expand and show more options.

When you make the window width narrower, the width of the sidebar changes(with a transition). I like the navbar idea, just wanted to point out the sidebar width changing when you change the width of the window(even by a small amount).

NOTMEE12 and others added 2 commits June 10, 2023 12:18
@NOTMEE12 NOTMEE12 marked this pull request as ready for review June 10, 2023 10:28
Copy link
Contributor

@Notenlish Notenlish left a comment

Choose a reason for hiding this comment

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

image

Fix the div with the class "related" taking up sidebar's space

Copy link
Contributor

@Notenlish Notenlish left a comment

Choose a reason for hiding this comment

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

image

Fix this(screenshot from iphone 12 pro screen resolution)

Copy link
Contributor

@Notenlish Notenlish left a comment

Choose a reason for hiding this comment

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

image

Can you replace the icon, doesn't fit well. It's too big and the white outline doesn't really make sense)

Copy link
Contributor

@Notenlish Notenlish left a comment

Choose a reason for hiding this comment

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

    transition-property: all;
    transition-duration: 2s;
    transition-timing-function: bezier;
    transition-delay: 0.1s;

I think you should get rid of the transition

Copy link
Contributor

@Notenlish Notenlish left a comment

Choose a reason for hiding this comment

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

image

Add cursor: pointer; for hovering over that part

Copy link
Contributor

@Notenlish Notenlish left a comment

Choose a reason for hiding this comment

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

div.header > div {
    /* border: {{ theme_headerborder }}; */
    border-collapse: collapse;
    background-color: {{ theme_headerbgcolor }};
    background-image: linear-gradient(to bottom, {{ theme_headerbgcolor}}, {{ theme_bgcolor }});
}
.dark-theme div.header > div {
    background-color: {{ theme_dark_headerbgcolor }};
    border: {{ theme_dark_headerborder }};
    background-image: linear-gradient(to bottom, {{ theme_dark_headerbgcolor }}, {{ theme_dark_bgcolor }});
}
div.header .logo {
    background-color: {{ theme_logobgcolor }};
    background-image: linear-gradient(to bottom, {{ theme_logobgcolor }}, {{ theme_headerbgcolor }});
    padding: 0.3em;
    /* border-right: 3px solid black; */
    display: flex;
    align-items: center;
    justify-content: center;
    flex-direction: column;
}
.dark-theme div.header .logo {
    background-color: {{ theme_dark_logobgcolor }};
    background-image: linear-gradient(to bottom, {{ theme_dark_logobgcolor}}, {{ theme_dark_headerbgcolor }});
    border: none;
}

Get rid of background-image. I don't think a gradient is needed, just using a color should be fine.

Copy link
Contributor

@Notenlish Notenlish left a comment

Choose a reason for hiding this comment

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

function toggleSectionVisibility(section, message){
	document.getElementById(section).hidden = ! document.getElementById(section).hidden;
	if (document.getElementById(section).hidden === true){ // show
		items = localStorage.getItem('hidden');
		if (items !== null){
			if (!(items.includes(section))){
				items = items + section + '|';
			}
			localStorage.setItem('hidden', items);
		} else {
			localStorage.setItem('hidden', section + '|');
		}
		document.getElementById(section[0]).innerHTML = document.getElementById(section[0]).innerHTML.replace('▼', '▲');
	} else {  // hide
		items = localStorage.getItem('hidden');
		if (items !== null){
			if (items.includes(section)){
				items = items.replace(section + '|', '');
			}
			localStorage.setItem('hidden', items);
		}
		document.getElementById(section[0]).innerHTML = document.getElementById(section[0]).innerHTML.replace('▲', '▼');
	}
}

function toggleChecked(classname){
    if (window.innerWidth <= 1100) {
        el = document.getElementsByClassName(classname)[0];
        console.log(el.id);
        if (el.id !== 'clicked') {
            el.id = 'clicked';
            htmlElement.classList.add('clicked');
        } else {
            el.id = null;
            htmlElement.classList.remove('clicked');
        }
    }
}

Do we have to save the hidden/closed tabs in localStorage? I don't think resetting hidden parts would create any inconvenience for the user.

Copy link
Contributor

@Notenlish Notenlish left a comment

Choose a reason for hiding this comment

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

function toggleSectionVisibility(section, message){
    ...
}

message argument isn't used

@NOTMEE12
Copy link
Author

image

How did you get that. What are the steps to replicate this?

@cool-dev-guy
Copy link

cool-dev-guy commented Jun 11, 2023

image

Can you replace the icon, doesn't fit well. It's too big and the white outline doesn't really make sense)

What about using unicode?<p>&#8801; </p> for hamburger menu?instead of svg?
eg :

@cool-dev-guy
Copy link

cool-dev-guy commented Jun 11, 2023

	box-shadow: 20px 10px 15px 0px rgba(0, 0, 0, 0.2);
	margin: 0;
	top: 0;
	height: 100vh;
	transition-property: all;

use 100% instead of 100vh

Is it working,cuz if the body&html is not 100% it cant take the entire window size.

Edit:

I checked the code and found that html is 100vh and body is 100% and div.header is 100vh,So the problem i mentioned above dosen't matter now.

@NOTMEE12 NOTMEE12 requested a review from Notenlish June 16, 2023 11:04
@NOTMEE12 NOTMEE12 requested a review from andrewhong04 June 17, 2023 09:31
Copy link
Contributor

@Notenlish Notenlish left a comment

Choose a reason for hiding this comment

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

image
It's hard to see the sidebar in light theme

input[name="q"]{
:-webkit-border-radius: 20px;
-moz-border-radius: 20px;
border-radius: 20px;
Copy link
Contributor

Choose a reason for hiding this comment

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

image
border-radius seems to be supported pretty much everywhere, why do you use -mox-border-radius and :-webkit-border-radius ?

Copy link
Author

Choose a reason for hiding this comment

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

should I remove it?

Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

Thanks for having a go at doing this.

My three thoughts when looking it over:

  1. I don't like the extra scroll bar that I get on the left hand side when all the submodule sections are open at the same time. I think that a) the submodules should perhaps start closed, only expanding when you click on the category and b) all the other sections should close when you expand one of them. This is how the default read the docs theme works, you can experiment with it in the Pygame GUI docs here.
  2. The upwards pointing arrow icons on the drop downs are confusing, generally I think the convention is that the arrows point to the right until expanded and then point down.
  3. I like the idea of broad categorisations for grouping the submodules, but since they are being changed from the classic trio of 'Most useful', 'Advanced' and 'Other'. There needs to be a bit of debate over the broad categories here. For instance, I would call the one labelled 'Music' here 'Sound' because it covers sound effects as well as playing music or midi tracks. The drawing section has too much stuff in it - including some very rarely used submodules alongside frequently used ones. time does seem to belong under window, and we also have a new submodule and class on the way called 'window' and Window. I'd be tempted to seperate out font and freetype into a section called 'Text'. 'touch' should definitely be in the input section. 'BufferProxy', 'PixelArray', 'pixelcopy' and surfarray could all go in the 'Other' bucket imho. I'm sure there are other changes that could be made but those would be a decent start, and many others will have opinions.

@MyreMylar
Copy link
Member

What I'm talking about with the drop down arrows btw (I've slightly edited a screenshot of the docs built on this PR):

image

You'll have to click though on the image to make out the arrows.

@NOTMEE12
Copy link
Author

  1. I don't like the extra scroll bar that I get on the left hand side when all the submodule sections are open at the same time. I think that a) the submodules should perhaps start closed, only expanding when you click on the category and b) all the other sections should close when you expand one of them. This is how the default read the docs theme works, you can experiment with it in the Pygame GUI docs here.

You mean that:

  • all sections should closed by default
  • only one section can be open at a time
  1. The upwards pointing arrow icons on the drop downs are confusing, generally I think the convention is that the arrows point to the right until expanded and then point down.

replace that with an arrow pointing to the right. Ok got it.

  1. I like the idea of broad categorisations for grouping the submodules, but since they are being changed from the classic trio of 'Most useful', 'Advanced' and 'Other'. There needs to be a bit of debate over the broad categories here. For instance, I would call the one labelled 'Music' here 'Sound' because it covers sound effects as well as playing music or midi tracks. The drawing section has too much stuff in it - including some very rarely used submodules alongside frequently used ones. time does seem to belong under window, and we also have a new submodule and class on the way called 'window' and Window. I'd be tempted to seperate out font and freetype into a section called 'Text'. 'touch' should definitely be in the input section. 'BufferProxy', 'PixelArray', 'pixelcopy' and surfarray could all go in the 'Other' bucket imho. I'm sure there are other changes that could be made but those would be a decent start, and many others will have opinions.

So here:

  • "Music" change to "Sound"
  • font and freetype to section "Text".
  • touch to the "input" section.
  • readd the "Other" section and put in it some not frequently used modules (like BufferProxy, PixelArray, pixelcopy)

Did I get everything right?

@MyreMylar
Copy link
Member

Thanks for sorting out the arrows 👍

  • all sections should closed by default
  • only one section can be open at a time

Correct. It should eliminate the double scrollbar issue and will also make things less cluttered when you first arrive.

So here:

  • "Music" change to "Sound"
  • font and freetype to section "Text".
  • touch to the "input" section.
  • readd the "Other" section and put in it some not frequently used modules (like BufferProxy, PixelArray, pixelcopy)

Did I get everything right?

Yep pretty much - though the 'Other' section is still there at the minute (at least on the latest version of this branch) so no need to readd it:

image

I think the grouping of submodules is a good idea, I just expect the exact organisation of submodules to take up the most time in getting this PR merged as I expect everyone with a stake in pygame-ce will have an opinion on it 😄

@NOTMEE12
Copy link
Author

hmmm... Yes the input section.

Now that I think about it, it should contain:

  • touch
  • controller
  • joystick
  • key

Which would leave the "events" section with only one item in it being the "event"

Should it be that way?

@zoldalma999
Copy link
Member

I am not sure whether I agree with this PR or not. On one hand I like the new sections, as they make more sense than just how useful they are (which is arbitrary anyway). And the looks of the side panel isn't bad either.

On the other hand, I don't like that now you can't see all the modules at a glance, and modules are one more click away (I used the top bar a fair amount, and this makes it a bit more "effort")

Anyway this is what I came up with for the sections:

Sections
essential (/core?)
-----
display
event
time
pygame (This could be in other?)
Surface (I put it in Surface manipulation, but it fits here too. Same with Rects)


Useful types (needs a better name, but I think it makes sense to put these together)
-----
Color
math
Rect
Mask


input
-----
key
mouse
joystick
controller
touch
camera (with a bit of a stretch, probably would go to other instead)


sound
-----
mixer
music
midi
sndarray


drawing and text
-----
draw
gfxdraw
font
freetype


surface manipulation
-----
Surface
transform
image
PixelArray
pixelcopy
surfarray
BufferProxy


system info
-----
system
scrap
cursors


experimental
-----
for not yet released modules (video, touch, controller, geometry, window) could
be here? Anything that has progress but is not yet ready to be released fully


other
-----
sprite
locals
version
examples
tests

@MyreMylar
Copy link
Member

Looks like I can't resolve merge conflicts or merge with main on this PR - probably because it is from NOTMEE12's main branch.

I'm going to switch it to draft due to the inability to fix it up and lack of agreement on the website changes.

@MyreMylar MyreMylar marked this pull request as draft December 16, 2023 07:45
@zoldalma999
Copy link
Member

Seems like this PR has been superseded by #2924.

@zoldalma999 zoldalma999 closed this Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants