bourgeoisor / xivtodo

Dashboards, tailored checklists and tools for Final Fantasy XIV.
https://xivtodo.com
GNU General Public License v3.0
122 stars 19 forks source link

Added support for "WikiUrl" on duty list items #68

Open acapper opened 7 months ago

acapper commented 7 months ago

image Added method of displaying information links on duty items. If no "WikiUrl" is found the behaviour should be the same as before.

I believe the information in frontend/src/assets/db.json will also require adjustment but it seems to be generated using scripts/csv_to_json.py which uses csv files that arent included in the repo and I dont see an obvious way to generate them with the available files.

bourgeoisor commented 7 months ago

Hi @acapper, thank you for your contribution!

I see a lot of structure changes that is probably not needed. The style changes also messes up some things, like the hover state which has unintended black text. No style diff should be necessary for this feature.

I spent a few minutes messing around with this and came up with a low-code-diff solution for this new icon:

image

image

Perhaps you could start from there? The only thing that's needed on top of that would be the conditional stuff you've already implemented, actually using the URL, etc.

This is the code in green, in my diff above, feel free to copy-paste:

      <div class="d-inline-flex align-items-center">
        <span
          v-if="duty.IsMSQ"
          class="icon-marker-msq"
          data-bs-toggle="tooltip"
          data-bs-placement="top"
          :title="$t('encounters.msqContent')"
        ></span>
        <span
          v-if="duty.Expansion && showPatchNums"
          :class="'icon-exp-' + duty.Expansion"
          data-bs-toggle="tooltip"
          data-bs-placement="top"
          :title="$t('encounters.unlockedInExp')"
        ></span>
        <a v-if="duty.WikiUrl" href="#" class="ms-2 text-success tt">
          <i class="fa-fw fad fa-external-link"></i>
          <span class="tt-text">Wiki resource</span>
        </a>
      </div>

Once you have a cleaner PR I'm happy taking another look, let me know! :)

P.S.: Yeah the db.json file is generated from a spreadsheet I built, which is not in this repo. I'm happy to make the DB changes for you!

acapper commented 7 months ago

What you have was the first approach that I took but you have this flex container wrapped in the click / hover div which means that those events are propagated when clicking / hovering over the link element. The other issue with your structure choice here is that due to the padding being on the list element means that the link element you have would be smaller and more difficult to increase in size without increasing the size of each line.

These examples both use your proposed change: This example shows the hover event propgating on the check box icon. image

Adding additional padding on the external link icon to make it easy to click will increase the row size which I was trying to keep the same. image

This is the result with my changes: image

image

Happy to compromise somewhere in the middle just explaining my resoning for the decisions made. Also I dont see the black text in the hover state you mentioned?

bourgeoisor commented 7 months ago

(Thank you for the detailed communication, btw, I appreciate it!)

Overall I'm happier with as small of a diff as possible; it would be great if you could start from the diff I shared (modifying it with the @click.stop of course, and with the proper conditionals and href and other things I'm missing--

Let me know if you have any other concerns, I'm happy chatting

acapper commented 7 months ago

I think @click.stop prevents the a href working at least from my testing. Also I dont see that black text I'm using Firefox might be Chrome specific?

I've reworked the entire changelist based on your example. Had to add a bunch of events to different refs which was the one of the reasons I didnt do it before. I think its better now but let me know if you can think of a good solution for the event handlers.

acapper commented 7 months ago

Sorry got confused along the way @click.stop is fine its the hover one that isnt since the link icon is techincally "outside" even though its a child.

acapper commented 7 months ago

I'm just stupid using hover instead on mouseover / mouseout. Should be all fixed 🤞

acapper commented 7 months ago

After thinking about this a little more I think I have a good solution for the mouse over events by tracking and extra bit of state. Had to move the events to the flex container to avoid pixel flicker caused by list item border.

Hope you like this better as adding more events wasnt the best approach 😁