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

New: Added static tooltips #304

Merged
merged 8 commits into from
Jun 11, 2024
Merged

New: Added static tooltips #304

merged 8 commits into from
Jun 11, 2024

Conversation

oliverfoster
Copy link
Member

@oliverfoster oliverfoster commented Apr 30, 2024

fixes #303

New

  • Added _useStaticTooltips and item._tooltip._position for labelling pins

Testing

  1. Use core issue/528
git clone https://github.com/adaptlearning/adapt_framework 528-static-tooltips
cd 528-static-tooltips
npm install && adapt devinstall
cd src/core
git checkout issue/528
cd ../../src/components/adapt-contrib-hotgraphic
git checkout issue/303
cd ../../..
grunt dev
  1. Edit the hotgraphic component to add
{
    "_hasStaticTooltips": true,
    "_isNarrativeOnMobile": false,
    "_items": [
      {
       "_tooltip": {
          "_isEnabled": true,
          "title": "Name",
          "_position": "top"
          
          "_COMMENT_position": "can be any combination of top left right bottom"
        }
      }
    ]
}
  1. Run grunt dev

It should look like this:
image
image

@oliverfoster oliverfoster self-assigned this Apr 30, 2024
less/hotgraphic.less Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

@oliverfoster
Copy link
Member Author

oliverfoster commented May 14, 2024

@swashbuck would you be able to do the schemas and readme and examples etc?

The core issue also needs +1 & review adaptlearning/adapt-contrib-core#529

@swashbuck
Copy link
Contributor

@swashbuck would you be able to do the schemas and readme and examples etc?

@oliverfoster Yep, done.

Copy link
Member Author

@oliverfoster oliverfoster left a comment

Choose a reason for hiding this comment

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

👍 +1

Copy link
Contributor

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

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

Great addition thanks. This worked as expected when testing on Mac Chrome, FF and iPhone.

When testing on Mac Safari, no tooltips were displayed when "_hasStaticTooltips": true. This worked as expected however when "_hasStaticTooltips": false.
Running Safari v17.5, macOS Sonoma v14.5

properties.schema Outdated Show resolved Hide resolved
schema/component.schema.json Outdated Show resolved Hide resolved
@oliverfoster
Copy link
Member Author

When testing on Mac Safari, no tooltips were displayed when "_hasStaticTooltips": true. This worked as expected however when "_hasStaticTooltips": false. Running Safari v17.5, macOS Sonoma v14.5

Re not working on Mac Safari, I'll contact Kirsty separately.

@swashbuck
Copy link
Contributor

When testing on Mac Safari, no tooltips were displayed when "_hasStaticTooltips": true. This worked as expected however when "_hasStaticTooltips": false. Running Safari v17.5, macOS Sonoma v14.5

Re not working on Mac Safari, I'll contact Kirsty separately.

@oliverfoster I can reproduce this, too. It looks like .tooltip__container isn't present in the DOM in Safari.

@oliverfoster
Copy link
Member Author

TooltipView is instantiated before Adapt loads fully.
https://github.com/adaptlearning/adapt-contrib-core/blob/4b8a99ab60704bb6d6993992ceae575b86a120c7/js/tooltips.js#L3

Where it is injected into the body.
https://github.com/adaptlearning/adapt-contrib-core/blob/4b8a99ab60704bb6d6993992ceae575b86a120c7/js/views/TooltipView.js#L92

Odd that it's missing. If you turn off static tooltips is it there? Are the navigation bar tootips appearing? How did you figure out the element isn't there? Do you have any console errors?

@swashbuck
Copy link
Contributor

swashbuck commented Jun 4, 2024

Odd that it's missing. If you turn off static tooltips is it there? Are the navigation bar tootips appearing? How did you figure out the element isn't there? Do you have any console errors?

Apologies, I just don't know how to read Safari's Web Inspector. It's there but is empty.

@swashbuck
Copy link
Contributor

swashbuck commented Jun 4, 2024

@oliverfoster If I load the page, navigate away to the menu, and then navigate back to the page with the Hot Graphic, the static labels appear. They are not there initially.

I also think I saw the static labels after reloading the page, but it's difficult to reproduce. Maybe 1 out of 100 reloads.

@oliverfoster
Copy link
Member Author

Could you remove this line and try that out please?
https://github.com/adaptlearning/adapt-contrib-core/blob/3ce4bd664535afc00c7924297359934d773777c6/js/views/TooltipView.js#L81

@kirsty-hames
Copy link
Contributor

Could you remove this line and try that out please? https://github.com/adaptlearning/adapt-contrib-core/blob/3ce4bd664535afc00c7924297359934d773777c6/js/views/TooltipView.js#L81

I can confirm this resolves the Safari issue reported above. I've retested in FF, Chrome and iPhone too.

@oliverfoster
Copy link
Member Author

Line removed here adaptlearning/adapt-contrib-core@e63be65

Copy link
Contributor

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

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

I have discovered a display issue with the initial rendering of the tooltip position. I'm not able to replicate every time but sometimes the top/bottom positioning is off. Please see screen shots below.

Tooltip labels position as expected:
tooltip_position_as_expected

Tooltip labels registering incorrect pin position:
tooltip_position_top_offset

Not sure if this is Mac specific but I'm able to replicate in Chrome, Safari and FF.

As noted above, I'm not able to replicate consistently. Sometimes the issue resolves itself either by resizing the browser or selecting a Hotgraphic pin.

Tooltip _position for the screen shot items are items 1 left, 2 right and 3 top.

@oliverfoster
Copy link
Member Author

oliverfoster commented Jun 5, 2024

Will have a look. Thanks.

It's suspiciously navigation bar sized.

@oliverfoster
Copy link
Member Author

oliverfoster commented Jun 10, 2024

@kirsty-hames / @swashbuck could you please try the associate pr as a resolution?

I could replicate and the pr seems to fix. adaptlearning/adapt-contrib-core#552

Copy link
Contributor

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

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

Safari issue resolved thanks @oliverfoster 👍

Copy link
Contributor

@joe-allen-89 joe-allen-89 left a comment

Choose a reason for hiding this comment

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

👍

@joe-allen-89 joe-allen-89 merged commit 7831ee3 into master Jun 11, 2024
@joe-allen-89 joe-allen-89 deleted the issue/303 branch June 11, 2024 09:01
github-actions bot pushed a commit that referenced this pull request Jun 11, 2024
# [6.13.0](v6.12.1...v6.13.0) (2024-06-11)

### New

* Added static tooltips (fixes #304) ([7831ee3](7831ee3)), closes [#304](#304)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add text labels to pins
5 participants