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

About page #128

Merged
merged 1 commit into from
Sep 14, 2021
Merged

About page #128

merged 1 commit into from
Sep 14, 2021

Conversation

bodamat
Copy link
Contributor

@bodamat bodamat commented Sep 4, 2021

Create about page #124
To add new people just update AboutPeople.csv

@bodamat
Copy link
Contributor Author

bodamat commented Sep 4, 2021

I forgot add button for mobile :)

@bodamat
Copy link
Contributor Author

bodamat commented Sep 4, 2021

Now all done :)

@HaSa1002
Copy link
Member

HaSa1002 commented Sep 5, 2021

I don't think a CSV table is the best suited data format for saving contributers.
I would list contributers in a markdown or text file. Also please use the names listed on the GitHub profile/used in commits as contributer name.

Copy link
Member

@HaSa1002 HaSa1002 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, just left a few ideas to improve the code

Comment on lines 389 to 363
[node name="Fam1" type="Label" parent="About/ColorRect/Control/ScrollContainer/VBoxAbout/BigThankYou/VBoxContainer"]
margin_right = 1138.0
margin_bottom = 45.0
custom_fonts/font = ExtResource( 5 )
text = "Mother"
align = 1
valign = 1
__meta__ = {
"_edit_use_anchors_": false
}

[node name="Fam2" type="Label" parent="About/ColorRect/Control/ScrollContainer/VBoxAbout/BigThankYou/VBoxContainer"]
margin_top = 49.0
margin_right = 1138.0
margin_bottom = 94.0
custom_fonts/font = ExtResource( 5 )
text = "Father"
align = 1
valign = 1
__meta__ = {
"_edit_use_anchors_": false
}

[node name="Fam3" type="Label" parent="About/ColorRect/Control/ScrollContainer/VBoxAbout/BigThankYou/VBoxContainer"]
margin_top = 98.0
margin_right = 1138.0
margin_bottom = 143.0
custom_fonts/font = ExtResource( 5 )
text = "Other members in my family"
align = 1
valign = 1
__meta__ = {
"_edit_use_anchors_": false
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is not necessary, but give more kind to the project :)

Copy link
Member

Choose a reason for hiding this comment

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

idk. I wouldn't do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in some project, I saw this, but of course not in commercial projects :)

src/project.godot Outdated Show resolved Hide resolved
@bodamat
Copy link
Contributor Author

bodamat commented Sep 5, 2021

I don't think a CSV table is the best suited data format for saving contributers.
I would list contributers in a markdown or text file. Also please use the names listed on the GitHub profile/used in commits as contributer name.

I also think about this, but easily and fast parse in Godot I use csv

@bodamat
Copy link
Contributor Author

bodamat commented Sep 5, 2021

Thanks @HaSa1002 for reviewing my code

@HaSa1002
Copy link
Member

HaSa1002 commented Sep 5, 2021

Thinking more about the format question, what about using a Resource? We can edit them in Engine and we don't need to parse them. Just create one like this:

class_name Authors
extends Resource

export(Array, String) authors := []
export(Array, Array, String) translators := [] # structure will be [[Name, Language]]

then just create the resource in the file system and assign it in your about script using

export var authors: Authors

@bodamat
Copy link
Contributor Author

bodamat commented Sep 5, 2021

I forgot about this idea, thanks. It should be much better. Do we need separate main developers and contributors?

@bodamat
Copy link
Contributor Author

bodamat commented Sep 5, 2021

Good idea, but this method also has disadvantages. This is so hard to update now, maybe in Godot 4.0, it changes. Can't change order position in Array and can't redo operation

@HaSa1002
Copy link
Member

HaSa1002 commented Sep 5, 2021

But the order doesn't matter?
Keep in mind, you can just edit the tres file by hand in a text editor

@bodamat
Copy link
Contributor Author

bodamat commented Sep 5, 2021

for language matter, but the only first index of array :) Dictionary is not enough to make also usable

src/about_authors.gd Outdated Show resolved Hide resolved
@HaSa1002
Copy link
Member

HaSa1002 commented Sep 5, 2021

and please rename about_authors.gd to authors.gd

@HaSa1002
Copy link
Member

HaSa1002 commented Sep 5, 2021 via email

@bodamat
Copy link
Contributor Author

bodamat commented Sep 5, 2021

yes, I want to group it. You can see this now in the last commit

@HaSa1002
Copy link
Member

HaSa1002 commented Sep 5, 2021

Okay then just do
grafik

@bodamat
Copy link
Contributor Author

bodamat commented Sep 5, 2021

Where do you get ideas? :)

Copy link
Member

@HaSa1002 HaSa1002 left a comment

Choose a reason for hiding this comment

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

You could iterate like this

export(NodePath) var developers_vbox
export(NodePath) var contributors_vbox
export(NodePath) var translators_vbox
export(Resource) var authours = authours as Authors
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export(Resource) var authours = authours as Authors
export(Resource) var authors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't do this, because I miss a hint from the editor about what Resource I want to use. I know it will fix in Godot 4.0

Copy link
Member

Choose a reason for hiding this comment

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

yeah but just fix the name (authors may work for hours, but the words are unrelated) and remove the assign. It is not needed. You could do

Suggested change
export(Resource) var authours = authours as Authors
export(Resource) var authors = null

and do assert(authors != null) in _ready

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but I will don't have autocomplete. Problem not about is null or not

Copy link
Member

Choose a reason for hiding this comment

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

But using the approach you used, you don't have autocompletion as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in code, I have, but not in the editor in inspector

@bodamat
Copy link
Contributor Author

bodamat commented Sep 5, 2021

How to use Resources created from class Author:

  • For Developers and Contributors just add a new item and add text. The first is a name and the second is GitHub name after -. If you don't know the real name just add a GitHub account.
  • For Translators add a new item what is Array. The first item in this Array is the language name and the other is people who help with translations. If you don't know the real name just add a GitHub account instead of the real name.

@HaSa1002
Copy link
Member

HaSa1002 commented Sep 5, 2021

Can you please squash all the commits into one?
You can either rebase or do a soft reset and create a new commit.

Rebase

git rebase HEAD~12 -i, then change the eleven last picks to for fixup.
git push -f, once done

Soft reset

git reset --soft HEAD~12
git commit -m "Add about page"
git push -f

@bodamat
Copy link
Contributor Author

bodamat commented Sep 7, 2021

I merged commits. It was pretty hard

Copy link
Member

@HaSa1002 HaSa1002 left a comment

Choose a reason for hiding this comment

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

I did a quick test and it works fine. Just one thing left to ensure it works on all screens

Copy link
Member

@HaSa1002 HaSa1002 left a comment

Choose a reason for hiding this comment

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

Nice work

@Jean28518
Copy link
Member

Very nice work!

Thank you!

@Jean28518 Jean28518 merged commit 485780e into Libre-TrainSim:master Sep 14, 2021
@HaSa1002 HaSa1002 mentioned this pull request Sep 16, 2021
@nalquas nalquas added the ux Usability and Expierience issues. label Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ux Usability and Expierience issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants