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

Add a ScrollBar to the skilllist #19

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

Ewwweee
Copy link

@Ewwweee Ewwweee commented Nov 25, 2023

feat(EventualSkills) : add a scrollbar to the list of skills in the menu

The scrollbar appears only if the height of the skills exceed the height of the window

It was made to resolve the issue #1 .

Ewwweee and others added 3 commits November 25, 2023 18:14
feat(skillList) : add a ScrollableArea to the list of the skills

The scrollBar is invisible until there are enough skill in the skill Menu (when the list of the buttons take too much place, a scrollbar appears)

It was made to resolve the issue #1
Update EventualSkillsTraining.ui
@jdrueckert
Copy link
Member

@Ewwweee it seems like someone committed to your fork. Is "Ihebsmai" someone you're familiar with?
In any case the file they added needs to be removed again before we can consider merging this.

@Ewwweee
Copy link
Author

Ewwweee commented Nov 29, 2023

Hello, I modified my pull request (my friend commit a change in the wrong repo). Do I need to open a new pull request ?

@jdrueckert
Copy link
Member

Hi @Ewwweee,
I just tested your PR in-game and that's what the result looks like:
image

Most problematic here is that when I reduce my window size, I cannot even see the skill buttons anymore:
image

IIRC what you shared on Discord looked different (and way better) - not sure what happened/changed? 🤔
Anyway, in this state I don't think we can merge this so please revisit your code changes.

@jdrueckert jdrueckert marked this pull request as draft December 6, 2023 10:17
@Ewwweee Ewwweee marked this pull request as ready for review December 10, 2023 14:20
@Ewwweee
Copy link
Author

Ewwweee commented Dec 10, 2023

Hello ! I fixed the pull request so normally it should work this time (never 2 without 3) ! . I added some skills to test the scrollbar in EventualSkills\prefab. It should look like this image if I correctly did my pull request.
image

@jdrueckert
Copy link
Member

Nice! Now it works perfectly fine 👍
Please remove the dummy skills you added for testing again and then we can get this merged 😃

@Ewwweee
Copy link
Author

Ewwweee commented Dec 12, 2023

Hello, thanks for your feedback. I removed the dummy skills so normally it should be ok.

@jdrueckert
Copy link
Member

Sorry for the delay, between being sick and the holidays, time just flew by 🙈

I just gave this another test and noticed the following when changing my window size:
image
As per your changes the skill learning screen has fix dimensions (height and width), causing it to not reduce its height (or width) if the window is smaller.
However, this issue is independent of the screen size being oriented at the number of skills which was resolved using the scrollbar, so I'd argue we can track this in a follow-up issue.

I still need to request two changes before merging, though:

  1. please adjust your PR title, as per our contribution guidelines, we try to follow conventional commits
    (please note that this is not necessary for every commit of your PR as we squash-merge it, because that results in a single commit on the module's develop branch based on your PR's title)
  2. please remove the logback-test.xml file, we don't need that checked-in to avoid the module behaving differently than the others. if we want to change the current default, this should be done generally for all modules.

@jdrueckert jdrueckert marked this pull request as draft April 7, 2024 18:49
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.

3 participants