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

[CLA on File] fix: [Safari] Time conductor ticks (#3537) #6033

Closed
wants to merge 2 commits into from
Closed

[CLA on File] fix: [Safari] Time conductor ticks (#3537) #6033

wants to merge 2 commits into from

Conversation

papo1011
Copy link

@papo1011 papo1011 commented Dec 2, 2022

mct_safari_time_conductor.mov

Closes #3537

Describe your changes:

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Command line build passes?
  • Has this been smoke tested?
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?
  • Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
  • Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Merging #6033 (e391259) into master (9ae58f8) will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6033      +/-   ##
==========================================
+ Coverage   54.66%   54.75%   +0.08%     
==========================================
  Files         600      600              
  Lines       23940    24041     +101     
  Branches     2115     2103      -12     
==========================================
+ Hits        13087    13163      +76     
- Misses      10254    10280      +26     
+ Partials      599      598       -1     
Flag Coverage Δ *Carryforward flag
e2e-ci 39.49% <ø> (ø) Carriedforward from 0e829cc
e2e-full 51.23% <ø> (ø) Carriedforward from 0e829cc
e2e-stable 50.06% <ø> (-0.09%) ⬇️
unit 50.34% <ø> (+0.06%) ⬆️ Carriedforward from 0e829cc

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
src/tools/url.js 92.59% <0.00%> (-3.71%) ⬇️
...s/telemetryTable/collections/TableRowCollection.js 42.93% <0.00%> (-3.27%) ⬇️
src/plugins/charts/bar/BarGraphView.vue 54.49% <0.00%> (-3.00%) ⬇️
...ugins/notebook/monkeyPatchObjectAPIForNotebooks.js 18.47% <0.00%> (-2.51%) ⬇️
src/plugins/plot/configuration/PlotSeries.js 78.15% <0.00%> (-2.30%) ⬇️
...c/plugins/persistence/couch/CouchObjectProvider.js 44.20% <0.00%> (-1.78%) ⬇️
src/plugins/imagery/components/ImageryView.vue 43.58% <0.00%> (-0.17%) ⬇️
src/plugins/notebook/components/Notebook.vue 20.43% <0.00%> (-0.16%) ⬇️
src/plugins/plot/MctPlot.vue 52.77% <0.00%> (ø)
src/ui/components/ToggleSwitch.vue 0.00% <0.00%> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ae58f8...e391259. Read the comment docs.

@ozyx ozyx added the source:community Community contribution or request label Dec 2, 2022
@ozyx ozyx changed the title fix: [Safari] Time conductor ticks (#3537) [CLA Needed] fix: [Safari] Time conductor ticks (#3537) Dec 2, 2022
@ozyx ozyx changed the title [CLA Needed] fix: [Safari] Time conductor ticks (#3537) [CLA Pending] fix: [Safari] Time conductor ticks (#3537) Dec 2, 2022
@jvigliotta jvigliotta changed the title [CLA Pending] fix: [Safari] Time conductor ticks (#3537) [CLA on File] fix: [Safari] Time conductor ticks (#3537) Dec 6, 2022
@ozyx ozyx added this to the Target:2.1.5 milestone Dec 7, 2022
@shefalijoshi
Copy link
Contributor

There seem to be more issues with safari - tick text doesn't appear even generally.

@ozyx ozyx self-requested a review January 18, 2023 18:33
@ozyx
Copy link
Member

ozyx commented Jan 18, 2023

There seem to be more issues with safari - tick text doesn't appear even generally.

I'm seeing this too, but seeing different things depending on the time conductor range on reload (haven't made sense of it yet):

5 min between tick labels, nothing visible:
image

Here, a smaller range. Only affects labels which are divisible by 5 ?
image

Same thing here...
image

Note that this only happens on initial load. If we change the time conductor bounds, the ticks and labels will appear properly.

@unlikelyzero unlikelyzero removed this from the Target:2.1.6 milestone Jan 20, 2023
@shefalijoshi
Copy link
Contributor

@papo1011 Would you be able to look at the issues listed here? We do realize there's more than the original issue states, but a partial fix is probably not what we want to commit at this point.
Please let us know if we should take over the fix or if you would like to continue investigating it.

@papo1011
Copy link
Author

papo1011 commented Mar 4, 2023

Hi, today I looked at the issue a bit again and in my opinion the way to find the bug is to analyse the code added since 20/11/2020 concerning the time conductor, as the issue (#3537) did not describe the behaviour found by @ozyx

@shefalijoshi
Copy link
Contributor

Thanks @papo1011 I'll take a look and get back to you soon.

@shefalijoshi
Copy link
Contributor

I have updated the issue stating the full problem.
Please let us know if you would like to fix the entire issue or if you prefer to close this PR.

@papo1011
Copy link
Author

papo1011 commented Apr 27, 2023

Hi @shefalijoshi today I looked for the causes of the tick labels not visible on reload, I haven't found a good solution :(
We could close this PR IMO

@shefalijoshi
Copy link
Contributor

@papo1011 Thank you for contributing to Open MCT! We will close this PR for now but might reopen it if some fix becomes evident down the road.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
source:community Community contribution or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Safari Browser] Time conductor ticks disappear on hover
4 participants