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.

Sign up for GitHub

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

Jump to bottom

Focus ring for goals modal #1055

Merged
merged 5 commits into from
Jan 28, 2023
Merged

Focus ring for goals modal #1055

merged 5 commits into from
Jan 28, 2023

Conversation

kostmo
Copy link
Member

@kostmo kostmo commented Jan 26, 2023

Fixes #1054

Testing

./scripts/play.sh --scenario data/scenarios/Testing/795-prerequisite/795-prerequisite-mutually-exclusive.yaml

and

./scripts/play.sh --scenario data/scenarios/Tutorials/conditionals.yaml

@kostmo kostmo force-pushed the goals-modal-focus-ring branch from 6de6eb7 to 8083325 Compare January 26, 2023 03:16
@kostmo kostmo marked this pull request as ready for review January 26, 2023 03:17
@kostmo kostmo requested review from xsebek and byorgey January 26, 2023 03:17
byorgey
byorgey reviewed Jan 26, 2023
View reviewed changes
@kostmo kostmo force-pushed the goals-modal-focus-ring branch from ad2c5b3 to beb04df Compare January 26, 2023 08:31
byorgey
byorgey approved these changes Jan 27, 2023
View reviewed changes
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

Looks good to me!

We should also make the objectives clickable, just like inventory items are clickable, and while we're at it we can make it so clicking on the goal description will change focus as well. But we can do that in a separate PR.

@byorgey
Copy link
Member

byorgey commented Jan 27, 2023

Hmm, actually, I just noticed that now the scrollbar overwrites the rightmost column of text:

scrollbar

Compare to the main branch:

scrollbar-main

In this PR the scrollbar has somehow moved over two columns to the left. Looking at the code I'm not yet sure what causes this.

@kostmo
Copy link
Member Author

kostmo commented Jan 28, 2023

scrollbar overwrites the rightmost column of text

Seems to be fixed after fiddling with the padding a bit.

@restyled-io restyled-io bot mentioned this pull request Jan 28, 2023
Closed
@kostmo kostmo added the merge me Trigger the merge process of the Pull request. label Jan 28, 2023
@mergify mergify bot merged commit 17c2d8c into main Jan 28, 2023
@byorgey byorgey mentioned this pull request Jan 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byorgey byorgey byorgey approved these changes

@xsebek xsebek Awaiting requested review from xsebek

Assignees
No one assigned
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No longer possible to scroll long goals
2 participants
@kostmo @byorgey