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

Finished sketching out quests, misc improvements #25

Merged
merged 7 commits into from Oct 6, 2022

Conversation

davvolun
Copy link
Contributor

@davvolun davvolun commented Oct 2, 2022

  • Finished sketching out the main quests
  • Added URL link for ListType
  • Added MapLink option for Requirement

@Gobluebro Gobluebro self-requested a review October 2, 2022 22:41
@Gobluebro Gobluebro added the enhancement New feature or request label Oct 2, 2022
@Gobluebro
Copy link
Owner

I gotta say this is a really well done PR. I think it captures the spirit of the project itself and you basically followed how I made the project in the first place.

I will test it myself and get back to you

@davvolun davvolun mentioned this pull request Oct 3, 2022
Copy link
Owner

@Gobluebro Gobluebro left a comment

Choose a reason for hiding this comment

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

A lot to digest here but I think it's good/helpful changes. Thanks for finishing off the quests I have been dreading them. I'm not gonna double check if they are exactly correct like I normally would. They all seem to match the style of how I did previous quests so I'm happy with them.

data/quests.ts Outdated Show resolved Hide resolved
data/bosses.ts Show resolved Hide resolved
data/types.ts Show resolved Hide resolved
requirements: [
{
id: "b049cb65-103b-4811-b218-1bd13ffdd9e5",
description: "Talk to Varré near The First Step grace.",
mapLink: "https://eldenring.wiki.fextralife.com/Interactive+Map?id=457"
Copy link
Owner

Choose a reason for hiding this comment

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

see my other comment about this. We could probably just wrap the The First Step grace in a hyperlink and link to this map link. Could add some brackets if you prefer to draw more attention to the link itself. e.g. [The First Step grace]

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 do like having a separate "[Map Link]" it sort of matches the fextralife style (although they're inconsistent about [Map Link], [Elden Ring Map Link], etc.). But this would be a great case for just having "The First Step" portion linked. Maybe a superscript shortcut icon, idk.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm really not a fan of their [Map Link]. I think it works for them because wikis generally have a lot of hyperlinks just for random words to explain them if you didn't know what it was.

For this project we generally are just looking to directly link someone to something important.

For this example, we just want the location that we need to go to on the map and it can be the whole sentence or just the grace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I like the first answer here https://ux.stackexchange.com/questions/4636/external-links-whether-how-to-distinguishing-them-from-internal-links-and-to

Basically just put a small 'external link' icon at the end of an item. Given the scope of the project, I don't think it needs to be "explained" -- just throw in something relatively uncomplicated ([Map Link] was nice because it matched the wiki anyway, but I don't care enough either way). Given the scope of this project, I don't think it needs to be particularly "explained," whatever is chosen.

Why don't you specify what you want in an enhancement, and if I have the time, I'll see if I can get it done? I can pull out the current MapLink functionality entirely if you like, just let me know.

Copy link
Owner

Choose a reason for hiding this comment

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

I would probably agree more with the second answer from that page. It seems unnecessary unless we were showing the users two links way too close to each other like e.g.

[here is some information with a link] [and it also continues here with a separate link]
for a user it would be harder to know if it was 2 links. If we limited it to just specific words like "X grace" being a hyperlink etc then I don't really see a need for a [Map Link]

I still think we should remove the maplink part of the javascript object though. It might be more work but overall is just better to have more control over where links get placed.

data/types.ts Show resolved Hide resolved
data/quests.ts Outdated Show resolved Hide resolved
data/quests.ts Outdated Show resolved Hide resolved
data/quests.ts Outdated Show resolved Hide resolved
data/quests.ts Outdated Show resolved Hide resolved
data/quests.ts Outdated Show resolved Hide resolved
@davvolun
Copy link
Contributor Author

davvolun commented Oct 4, 2022

Good changes suggested, I should be able to get this PR updated later today.

@davvolun
Copy link
Contributor Author

davvolun commented Oct 5, 2022

I addressed most of the suggestions, I'm thinking it'll take just a bit longer to do the map link thing you suggested (and get it styled nicely). Maybe open it as an enhancement and I'll see what I can do with it?

@davvolun
Copy link
Contributor Author

davvolun commented Oct 5, 2022

Oh -- let me know if I missed anything.

data/quests.ts Outdated
Comment on lines 1280 to 1286
description: "Reload the area if necessary, then give a fifth Deathroot to Gurranq for the Beast Claw Incantation."
description: "Reload the area if necessary, then give a fifth Deathroot to Gurranq for the Beast Claw Incantation.",
url: "https://eldenring.wiki.fextralife.com/Deathroot"
Copy link
Owner

Choose a reason for hiding this comment

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

I meant more having a step of where to retrieve a deathroot from. Telling people to just turn in deathroots doesn't really tell the full story.

Found inside a chest behind the Black Knife Assassin in the [Deathtouched Catacombs].
Found inside a chest behind the Cemetery Shade in the [Black Knife Catacombs].
Defeat the Tibia Mariner at [Summonwater Village].
Defeat the Tibia Mariner at [Wyndham Ruins].
Defeat the Tibia Mariner at [East Liurnia of the Lakes].
Defeat the Tibia Mariner on a ledge between [Castle Sol Main Gate and Snow Valley Ruins Overlook]. 
Found inside a chest behind the Red Wolf of the Champion at [Gelmir Hero's Grave].
Found inside a chest after defeating the Stray Mimic Tear in the secret catacombs underneath the [Hidden Path to the Haligtree].
Found inside the chest after defeating the Ulcerated Tree Spirit boss inside the [Giants' Mountaintop Catacombs].

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OH! I gotcha now. Should be able to get that updated tonight or tomorrow.

@Gobluebro
Copy link
Owner

Gonna merge this and we can move the conversation to the other PR

@Gobluebro Gobluebro merged commit ca086cd into Gobluebro:main Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants