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

NG1675 - BarStacked/ColumnStacked Many Records #8705

Merged
merged 2 commits into from May 14, 2024

Conversation

tjamesallen15
Copy link
Contributor

Explain the details for making this change. What existing problem does the pull request solve?

This pull request will fix an error encountered when having many records inside the graph.

Related github/jira issue (required):
Closes infor-design/enterprise-ng#1675
Related #8367

Steps necessary to review your pull request (required):

Included in this Pull Request:

  • An e2e or functional test for the bug or feature.
  • A note to the change log.

@tjamesallen15 tjamesallen15 marked this pull request as ready for review May 9, 2024 12:04
@tjamesallen15 tjamesallen15 requested a review from a team as a code owner May 9, 2024 12:04
@tmcconechy
Copy link
Member

@tjamesallen15 maybe the tooltip needs reformatting in this case (two columns?) so maybe something where it goes two column if more then a certain no of records

Hover the bar

Screenshot 2024-05-09 at 10 28 46 AM

@tmcconechy tmcconechy added the ready for qa Ready for QA to review label May 9, 2024
Copy link
Contributor

@glenlieorillo glenlieorillo left a comment

Choose a reason for hiding this comment

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

Showing no errors in the console. However, I agree with @tmcconechy. The tooltip is not displaying correctly. Created a new issue for this bug: #8708

Screen.Recording.2024-05-10.at.12.57.27.PM.mov

image

Screen.Recording.2024-05-10.at.1.04.24.PM.mov

@n-ace-ancog
Copy link

Aside from the tooltip, the vertical scroll bar seems it overshot a little.

image

janahintal
janahintal previously approved these changes May 10, 2024
Copy link
Contributor

@janahintal janahintal left a comment

Choose a reason for hiding this comment

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

No errors on my end.
image
image

Copy link
Member

@tmcconechy tmcconechy left a comment

Choose a reason for hiding this comment

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

Looks good to me. I wouldnt mind fixing the tooltip issue if possible too but if we need to wait thats fine.

Also funny, i saw the scrollbar issue now i dont see it? Did you fix? I saw a fix for it yesterday but got side tracked, now i dont see it?

Copy link
Contributor

@janahintal janahintal left a comment

Choose a reason for hiding this comment

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

Tooltip still looks cut off in Mobile / Responsive view
image

@tmcconechy
Copy link
Member

@tjamesallen15 are we fixing the tooltip to two column? Or doing that later?

@ericangeles
Copy link
Contributor

@tmcconechy he's on PTO. For me, we can do it later. Am okay with the fix.

@tmcconechy
Copy link
Member

@ericangeles sure, i think maybe just change it to stack in two columns if say more than 10 bar items

@ericangeles
Copy link
Contributor

@tmcconechy, agree with that design.

Copy link
Contributor

@glenlieorillo glenlieorillo left a comment

Choose a reason for hiding this comment

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

Showing no errors in the console log. Tooltip issue: #8708

Screen.Recording.2024-05-14.at.3.17.37.PM.mov
Screen.Recording.2024-05-14.at.3.18.10.PM.mov

Copy link
Contributor

@janahintal janahintal left a comment

Choose a reason for hiding this comment

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

image

image

Copy link
Contributor

@jbrcna jbrcna left a comment

Choose a reason for hiding this comment

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

image image

@tmcconechy
Copy link
Member

@ericangeles should we maybe disable the tooltip? Or put in an issue to fix it later?

@ericangeles
Copy link
Contributor

Lets fix it later, Tim. I believe QA created the issue on tooltip.

@tmcconechy tmcconechy merged commit ab75dc1 into main May 14, 2024
2 checks passed
@tmcconechy tmcconechy deleted the NG1675-bar-column-stack-many-records branch May 14, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for qa Ready for QA to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bar/Column-Stacked: Displaying 21 records and above results to console error and missing legend
7 participants