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

[Research] OUI Compliance audit of the vis_type_tagcloud plugin #4100

Closed
BSFishy opened this issue May 22, 2023 · 4 comments
Closed

[Research] OUI Compliance audit of the vis_type_tagcloud plugin #4100

BSFishy opened this issue May 22, 2023 · 4 comments
Assignees
Labels
good first issue Good for newcomers OUI compliance Issues and PRs to maximize OUI usage and remove style and component hacks OUI Issues that require migration to OUI

Comments

@BSFishy
Copy link
Contributor

BSFishy commented May 22, 2023

Tracking issue for OUI compliance audit of the vis_type_tagcloud plugin.

Follow the steps in #3945 to complete the audit, then open another issue with findings. Once the work of performing the audit is done, this issue can be closed.

Related files
size (bytes) file notes
442 src/plugins/vis_type_tagcloud/public/components/tag_cloud.scss

If this issue isn't already assigned to someone and you'd like to take on this work, please ping @BSFishy in the comments and I can assign it to you.

@BSFishy BSFishy added OUI Issues that require migration to OUI and removed untriaged labels May 22, 2023
@seanneumann seanneumann moved this to Todo in Look & Feel May 23, 2023
@BSFishy BSFishy added the good first issue Good for newcomers label May 24, 2023
@joshuarrrr joshuarrrr added the OUI compliance Issues and PRs to maximize OUI usage and remove style and component hacks label Jun 1, 2023
@nurSaadat
Copy link

nurSaadat commented Jun 1, 2023

tag_cloud.scss has four custom classes.

tgcChart__container and tgcChart__wrapper

.tgcChart__container,
.tgcChart__wrapper {
flex: 1 1 0;
display: flex;
}

They are used in tag_cloud_chart.tsx to wrap a tag cloud component.

<EuiResizeObserver onResize={updateChartSize}>
{(resizeRef) => (
<div className="tgcChart__wrapper" ref={resizeRef}>
<div className="tgcChart__container" ref={chartDiv} />
</div>
)}
</EuiResizeObserver>

I suggest replacing div className="tgcChart__wrapper" by EuiFlexGroup.
Also, I suggest replacing div className="tgcChart__container" by EuiFlexItem. However, I found that EuiFlexItem doesn't have property ref, so maybe it can be added.

tgcChart

.tgcChart {
position: absolute;
top: 0;
left: 0;
width: 100%;
height: 100%;
overflow: hidden;
}

This class is used in tag_cloud_visualization.js

const cloudRelativeContainer = document.createElement('div');
cloudRelativeContainer.classList.add('tgcChart');
cloudRelativeContainer.setAttribute('style', 'position: relative');
const cloudContainer = document.createElement('div');
cloudContainer.classList.add('tgcChart');

I suggest element creation should be realized as a function. Otherwise, it is hard to substitute div by OUI component. The most important properties for chart's visual appearance are height: 100% and position: relative.

tgcChart__label

.tgcChart__label {
width: 100%;
text-align: center;
font-weight: $euiFontWeightBold;
}

This class is used to style the label of a tag cloud. Apparently, this label is not shown, as the property shouldShowLabel is never changed.

export class Label extends Component {
constructor() {
super();
this.state = { label: '', shouldShowLabel: true };
}
render() {
return (
<div
className="tgcChart__label"
style={{ display: this.state.shouldShowLabel ? 'block' : 'none' }}
>
{this.state.label}
</div>
);
}
}

Later, Label component is used as follows

this._labelNode = document.createElement('div');
this._containerNode.appendChild(this._labelNode);
this._label = React.createRef();
render(<Label ref={this._label} />, this._labelNode);

I suggest Label component can be substituted by OUI text component, or removed and put into metadata as aria label, for example.

Conclusion

  • Add ref property to EuiFlexItem
  • Substitute custom classes by OUI components
  • Refactor new DOM element creation
  • Remove Label component

@nurSaadat
Copy link

@joshuarrrr @BSFishy could you assign me to this issue?

@joshuarrrr
Copy link
Member

@nurSaadat Do you mind moving your audit findings to a new issue? Sorry for the extra administrative overhead, but it will help us track progress on the overall initiative easier.

@joshuarrrr joshuarrrr moved this from Todo to In Progress in Look & Feel Jun 8, 2023
@nurSaadat
Copy link

nurSaadat commented Jun 14, 2023

@joshuarrrr sure. Opened as #4285

@github-project-automation github-project-automation bot moved this from In Progress to Done in Look & Feel Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers OUI compliance Issues and PRs to maximize OUI usage and remove style and component hacks OUI Issues that require migration to OUI
Projects
Status: Done
Development

No branches or pull requests

4 participants