otc-cirdan / gw2pc

https://gw2pc.com/
GNU General Public License v3.0
3 stars 2 forks source link

New Items, refactored view classes into files, item icons, and updated/tested libraries #43

Closed NoahFrank closed 2 years ago

NoahFrank commented 2 years ago

Changes:

Questions:

Caveats:

Fix:

otc-cirdan commented 2 years ago

I'm getting some error about buckets from Zappa when I try to deploy anything, including the existing version of the code. I'll deploy this to staging and ask you to test it, but I need to figure out that issue first.

otc-cirdan commented 2 years ago

As it turns out, reading the error message explains the error message.

@NoahFrank , your code is live at https://dev.gw2pc.com/ , please test it and see if everything is working as intended.

I had to bump a few versions in etc/requirements.txt, please update your PR to take these into account. The old version of django-s3-storage isn't compatible with Django 4.0:

-zappa==0.53.0
+zappa==0.54.2
-django-s3-storage==0.13.4
+django-s3-storage==0.13.7

I'll review your pull request more substantively in the coming days.

otc-cirdan commented 2 years ago

I personally prefer opening a link in a new tab by default, so I experimented in this PR with this idea. Mixing the behavior of opening a new tab or redirecting to the link should be standardized as one or the other across the entire site. I can make these changes if you have an opinion, basically wanted to get your thoughts on the matter.

I do not think that links should open in a new tab by default. Middle-click / ctrl-click is a common shortcut to open a link in a new tab if the user wants to, but I think that opening links in a new tab by default is often surprising and undesired, and does not come with a commonly-known way for the user to see that the link will open in a new tab before clicking it, and to choose to open that link in the same tab if they desire.

There is maybe the example where the user wants to open the bltc links for a number of items at once to get some more detailed analysis. However, it's easy for users to hold down ctrl or use the middle button in that case, while it's not possible to force a link to open in the same tab if we choose to open a new tab by default.

Are the editorial market explanations tailored to each item required for my PR to be accepted? As it stands the editorialization I added is lacking.

For items that are very simple - if they routinely trade at 90% and there isn't much more to say about them - then it's fine to just say that. For Gen 3s I assume they are pretty similar to Gen 1s and the explanation can probably safely be copy/pasted.

NoahFrank commented 2 years ago

Looks like everything is working in the dev environment! I noticed an issue with my fix mentioned in the PR so I made another commit to address it.

I do not think that links should open in a new tab by default.

I have also remove the default opening in new tab behavior!

otc-cirdan commented 2 years ago

9c05aaa pushed to https://staging.gw2pc.com/ (and dev)

NoahFrank commented 2 years ago

Did you have a chance to review the changes in more detail? Any stability issues hosting the dev/staging servers?

otc-cirdan commented 2 years ago

I don't see any major issues, but there are a few additional dependency version updates that are needed which I mentioned in the review comments above

NoahFrank commented 2 years ago

Okay cool, I updated the dependency versions, let me know what else is needed before PR approval!