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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

acapper
Copy link

@acapper acapper commented Nov 16, 2023

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
Copy link
Owner

bourgeoisor commented Nov 17, 2023

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
Copy link
Author

acapper commented Nov 17, 2023

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
Copy link
Owner

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

  • For the parent click propagating, you can prevent this by adding a @click.stop="" to the <a> tag, like so:
<a href="#" class="ms-2 text-success tt" @click.stop="">
  <i class="fa-fw fad fa-external-link"></i>
  <span class="tt-text">Wiki resource</span>
</a>
  • For the size, I don't think it's a big issue, especially if you add an tooltip with zero delay like the diff I wrote earlier, since there's instant feedback that your mouse is on the button:
    image

  • As for the black text, this is what I see on-hover with your branch :)
    image

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
Copy link
Author

acapper commented Nov 17, 2023

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
Copy link
Author

acapper commented Nov 17, 2023

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
Copy link
Author

acapper commented Nov 17, 2023

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

@acapper acapper reopened this Nov 17, 2023
@acapper
Copy link
Author

acapper commented Nov 17, 2023

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 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants