Tsunder / Pardus-Sweetener

A browser extension that enhances the user interface of the online game Pardus
3 stars 3 forks source link

Updates #105

Closed Sei-lus closed 3 years ago

Sei-lus commented 4 years ago

Fix for:

14, #17, #30 (Fixed but not sure if by me or not), #97 , #98 , #99 , #100 . #101

Drugs message is now consistent with the Stim Message. Added a Title + Column Separator of "Display Missions" on Nav Screen Justification: As a new player, I had no idea what each column really meant until I read the code. Also added specific IDs to better CSS the table on Stylus. Quality of Life change to clock order: Drug and Stim clocks are now side by side. GMT renamed to UTC because I consider GMT really outdated. People need to get on the new terms. Put Server Reset and UTC as the last clocks since they happen once a day. Removed Repair Quicklink and Trade quicklink since Pardus includes it nowadays.

#93 Warning. I changed the timer to 0,29,3,0 in nav.js since I care more about Artemis and don't use drugs on the others 2. You may not want to merge that specific line. Manifest Warning. I changed the name of the addon to "Sweetner Artemis" and the version to test privately in Mozilla. But the fact it says Artemis is a good indicator that this also has the drug timer changed.

Tsunder commented 4 years ago

Can you split this into smaller pull requests and clean it up?

I am willing to accept these features, but it does need to be cleaned up a bit.

Sei-lus commented 4 years ago

Sorry for not cleaning it up... Since I'm new, I thought I needed to leave behind what was changed or removed for review. I can definitely clean it up.

But I don't really follow what you mean by making small pull request since most things get modified on the same .js file. Also, the manifest was changed to be able to play with my own "copy" for testing purposes. I will get this handled tomorrow.

Tsunder commented 4 years ago

I understand! Welcome to github, and "Version Control". It helps keep things organized.

By making smaller pull requests, I mean making separate branches for each feature, then submitting each branch separately as a pull request. You can have multiple branches and pull requests at one time. A branch is basically another version/copy of the code.

https://help.github.com/en/desktop/contributing-to-projects/creating-a-branch-for-your-work

Sei-lus commented 4 years ago

You can test the changes in here: https://addons.mozilla.org/firefox/downloads/file/3578007/pardus_sweetener_artemis-9.12.9-fx.xpi?src=devhub Any way of contacting you via Discord?

Tsunder commented 4 years ago

please split this into multiple, more digestible PRs. What I can see here is some bug fixes, some new features, some QoL changes, and some changes that need review/won't pass (like the drug tick change). As it is though, I cannot accept this PR like this

Tsunder commented 4 years ago

Some ways I'd probably split it...

mission board upgrades

It looks nice, but I think it takes up too much space per mission. It's okay when there's only a few missions, but each mission entry should take up I think 2 lines at most. Additionally, missions have timers that go over an hour. Adding something like a HH:MM:SS could be useful.

reordering of clocks

The reason why drugs/stims are rightmost is for visibility. That being said the drug clock and UTC clock should be swapped places so that they're together yes

various css things, the trade/repair links

seems good

drug tick switch

needs to be universe specific/an exception for Artemis. I don't think it's reasonable to make the clocks on 2/3 of the universes incorrect here.

overall: getting there, clearly a lot of code work has been put in, and I'd like to accept as much of it as possible.

I'm not entirely sure why #99 and #93 are mentioned

Sei-lus commented 4 years ago

Hi! Let me try to reply in a way that it makes sense why somethings are not to your liking.

"please split this into multiple, more digestible PRs."

As you mentioned, I put a lot of work (all at the same time) and since I'm an amateur... the truth is I don't know what I can erase from the new changes without breaking other stuff to make it several pull request. I was hoping you could figure that out.

"What I can see here is some bug fixes, some new features, some QoL changes, and some changes that need review/won't pass (like the drug tick change). As it is though, I cannot accept this PR like this"

The drug tick change isn't something I want to pass. You mentioned in the topic about that "bug" that you would make it in a way Artemis is different than the rest... I don't know enough Javascript to make it so that is up to you to fix or to teach me to fix. Meanwhile, I don't mean to break 2 universes, this is my own personal "Sweetner" since I only care about Artemis and I left that code there to remind you of it... It may not make sense to break 2 universes, but 1 has been broken forever and maybe someone that pass by could find it handy until you make the clever code fix.

"mission board upgrades It looks nice, but I think it takes up too much space per mission. It's okay when there's only a few missions, but each mission entry should take up I think 2 lines at most. Additionally, missions have timers that go over an hour. Adding something like a HH:MM:SS could be useful."

For timers that go over 1 hour: That is already implemented but in the way my brain thought I could handle it. Currently it takes the timer that are, for example, "1 hour and 10 minutes" and converts it to "70 minutes". For clutter: I mentioned that too in the bug report. Currently I think this is a cool thing to add but I do agree 100% that on the early ranks (i.e. rank 8 and earlier) AND maybe package ranking it is too cluttered. I would like to ask for a way to either let the pilot choose which one he wants or to make a code that looks for the rank/competence of the pilot and if below a threshold then eliminate the timer since it is unlikely to be needed. I don't know to be honest.

"reordering of clocks The reason why drugs/stims are rightmost is for visibility. That being said the drug clock and UTC clock should be swapped places so that they're together yes"

I will give feedback about the clock order as a new player and this is my current array: [ 'AP', 'P', 'S', 'B', 'L', 'E', 'N', 'Z', 'D','Stim', 'UTC', 'R'] We have clocks for 4 roles here. Trader, Ranker, Skiller and Universal. Universal is for every pilot = AP. I choose this to be at the left most side for coherency across. Trader = P, S, B, E. I choose this order because the way ticks happen is P > S > Nothing > P > S > B. Having Building in the middle always was distracting in that scheme and E is kinda unimportant for most traders. Skiller = L, N, Z, D, Stim, UTC. Self explanatory. Ranker = D, Stim, UTC. Self explanatory with a caveat... Mozilla uses anti fingerprinting so the UTC clock is important with the "mission display" that was added until players can manually choose their timezone in Sweetner People may be in more than one category across universes so I propose this order: AP (Universal), P, S, B, E (Trader and logical sequence, left side because it is not "life or death"), L, N, Z, D, Stim, UTC (Skiller + Ranker + Universal, UTC is important due to the minute mark for NPC attacks) and I don't find a purpose for R... But R.

"various css things, the trade/repair links seems good"

Thank you

"drug tick switch needs to be universe specific/an exception for Artemis. I don't think it's reasonable to make the clocks on 2/3 of the universes incorrect here."

Agree.

"overall: getting there, clearly a lot of code work has been put in, and I'd like to accept as much of it as possible."

Thank you again, by the way... I don't mean to impose or anything... I'm happy with my version of Sweetner right now as a individual pilot and wanted to contribute. I also don't feel bad if you don't accept anything and I'm not refusing to split it... it is just that I'm really amateur at this. I mostly did reverse engineer to pull what I did since my javascript knowledge was 0 prior to this.

"I'm not entirely sure why #99 and #93 are mentioned"

At the time, I didn't know you had added 99 already but I also did it, since then I changed some text to be more "IC" like the weapon link to "Armory" and such. 93 was a mistake, sorry.

Edit: I think I gave you full edit power over my pull request/branch.

Sei-lus commented 3 years ago

@Tsunder Hey, just a notice that I forked your project. It is up to date with yours, but it also has the fixes I made available in this pull request last year. I wanted to take the opportunity to allow for a "test" addon so you could see the changes in the pull request.