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

add wrapping for long text documents to text highlighting component in RAI dashboard for text #1643

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

imatiach-msft
Copy link
Contributor

Description

During testing it was found that large documents don't display well in the RAI text dashboard because there is no wrapping. This PR adds text wrapping to the text highlighting component to resolve this issue for long text documents and adds a test with a long text document.
I also refactored part of applications.ts file into a new file applicationInterfaces.ts as the applications.ts file was over 300 lines after adding the additional test case to the test app.

Before, without wrapping:

image

After, with wrapping:

image

Checklist

  • I have added screenshots above for all UI changes.
  • I have added e2e tests for all UI changes.
  • Documentation was updated if it was needed.

@codecov-commenter
Copy link

codecov-commenter commented Aug 16, 2022

Codecov Report

Merging #1643 (c43a2d1) into main (39c2a20) will increase coverage by 1.31%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1643      +/-   ##
==========================================
+ Coverage   89.01%   90.33%   +1.31%     
==========================================
  Files         105       39      -66     
  Lines        5489     1676    -3813     
==========================================
- Hits         4886     1514    -3372     
+ Misses        603      162     -441     
Flag Coverage Δ
unittests 90.33% <ø> (+1.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
raiwidgets/raiwidgets/explanation_dashboard.py
...sibleai/responsibleai/rai_insights/rai_insights.py
...bleai/responsibleai/_tools/causal/causal_result.py
...tabalanceanalysis/distribution_balance_measures.py
...nsibleai/databalanceanalysis/data_balance_utils.py
...eai/responsibleai/databalanceanalysis/constants.py
...ponsibleai/responsibleai/modelanalysis/__init__.py
...iwidgets/raiwidgets/fairness_metric_calculation.py
...ibleai/_tools/causal/dashboard_schemas/__init__.py
responsibleai/responsibleai/managers/__init__.py
... and 56 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -28,6 +35,7 @@ export const textHighlightingStyles: () => IProcessedStyleSet<ITextHighlightingS
{
color: getPrimaryChartColor(theme),
fontSize: theme.fonts.large.fontSize,
margin: "2px",
Copy link
Contributor Author

@imatiach-msft imatiach-msft Aug 16, 2022

Choose a reason for hiding this comment

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

When I added wrap to FluentUI Stack, it removed the margin for Labels but not Text, which squished the text with underlines together (but not the highlighted or plain text) - hence I needed to add the margin here. I wonder if it might just be better to always use FluentUI Text components and sometimes set fontweight instead of using FlunetUI Label components. This might be a good discussion point to bring up with Ke when he is back, since he preferred to use Label for bold words.

@imatiach-msft imatiach-msft merged commit bf4f019 into main Aug 17, 2022
@imatiach-msft imatiach-msft deleted the ilmat/fix-text-wrapping branch August 17, 2022 19:00
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.

3 participants