Skip to content

Conversation

@JayPea00101010
Copy link
Contributor

@JayPea00101010 JayPea00101010 commented Oct 14, 2019

I think this works as a request, (fingers crossed)
As well as adding the XML for the subraces I've added a .index file for races from r/UA

@JayPea00101010
Copy link
Contributor Author

for this file to work properly the following pull request also needs to be approved
aurorabuilder/elements#384

@JayPea00101010
Copy link
Contributor Author

aurorabuilder/elements#351
closes this content request

@JayPea00101010
Copy link
Contributor Author

typos and not changing files strikes again
I've fixed those now though and also fixed not putting .xml in the file location

Copy link
Contributor

@swdriessen swdriessen left a comment

Choose a reason for hiding this comment

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

Be sure to test the files before you submit the pull request, this makes it easier for the person who needs to check and test it.
I'd suggest you create characters for the content you create at a high enough level of the highest fetaure in the content (level 3 in this case). Then when you generate a sheet you should see all the features / traits that you expect.

@NickVendel
Copy link

Okay, i have one issue (for now) with this pull, why do you put Reddit: DnD inside reddit-unearthed-arcana.index?

@JayPea00101010
Copy link
Contributor Author

I've now fixed that

@ColdAnkles
Copy link
Contributor

So, on the filename - I think it should be probably "dwarf-TheArenaGuy.xml".
Prepending it with subrace is redundnant, so I think mentioning dwarf makes sense.
Unless you split them in to individual files for each subrace? "dwarf-jungle.xml" & etc.
Ultimately all it needs is to be is clear where a subrace is coming from so if a file needs updating due to errors or changes to Aurora, its obvious which file is for what on name alone.

@JayPea00101010
Copy link
Contributor Author

That makes sense, I was avoiding having them as multiple files given its all th same source though i guess that would work too.
I'll change it to "dwarf-TheArenaGuy.xml" when I can

@JayPea00101010
Copy link
Contributor Author

ok pretty sure I've changed it where it needs changing, probably helpful for someone else to double check it though

@NickVendel
Copy link

NickVendel commented Oct 31, 2019

Prepending it with subrace is redundnant, so I think mentioning dwarf makes sense.
Unless you split them in to individual files for each subrace? "dwarf-jungle.xml" & etc.

I have few things to say about that:

  1. there's nothing wrong with doing subraces-dwarf.xml, even if you can't find subraces inside original repository.

Ultimately all it needs is to be is clear where a subrace is coming from so if a file needs updating due to errors or changes to Aurora, its obvious which file is for what on name alone.

  1. I find this point incredibly useless, since when you get an error on app loading, it will usually point to the file that has issues (alternatively it will not point at any file at all). And if you get errors while using it, error will not display name of the file anyway.
    File has a url to point where it coming from. You could also force the use of "url setters", that will link elements and url links under description in the app to proper source, rather than just throwing people to Reddit or different subreddits.
  2. If you say it should be clear, and you want to use this structure of [race/class/etc.]-[author].xml, then you should do it for already existent content too, so to not hold other people to different standards of structuring. And to prevent a mess of people doing different things in different folders.

@JayPea00101010
Copy link
Contributor Author

ok so I've now updated the file name again, should all be good now

Copy link
Contributor

@ColdAnkles ColdAnkles left a comment

Choose a reason for hiding this comment

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

I think this is fine to go now. Anyone else have comments/suggestions?

@ColdAnkles
Copy link
Contributor

Tried to remove the u-a change to stop it from appearing in this PR, no such luck :(

@JayPea00101010
Copy link
Contributor Author

tried to remove the what?

@ColdAnkles
Copy link
Contributor

"files changed" shows that " reddit/reddit-unearthed-arcana.index " was edited to remove a line ending... or add one? unrelated to the PR since you changed it to r/DnD

@JayPea00101010
Copy link
Contributor Author

ok so in trying to fix it I've completely screwed it up

@JayPea00101010
Copy link
Contributor Author

note to self, never try to fix things like this again

@JayPea00101010
Copy link
Contributor Author

it's good now though

@ColdAnkles
Copy link
Contributor

I make more mistakes attempting to fix something than I make breaking things in the first place. Will merge soon if nobody objects

@ColdAnkles
Copy link
Contributor

I had a thought, make it subraces-dwarf.xml instead so it ends up grouped with other subrace files (if any)

@JayPea00101010
Copy link
Contributor Author

Don't push it yet anyway, I think swd said ungrant was coming next version and as this is at the moment it requires a push to the main pull, since the next version is coming fairly soon it's probably better to wait until that's a thing

@JayPea00101010
Copy link
Contributor Author

and I'll change the name when I change it with ungrant because I'm lazy so there's no more unnessesary commits

@ColdAnkles ColdAnkles added the pending aurora Waiting for an update to the Aurora itself. label Dec 7, 2019
@ColdAnkles
Copy link
Contributor

Created a label specifically for pulls that require updates to Aurora itself before they get merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pending aurora Waiting for an update to the Aurora itself.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants