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

Added a Figma specific PR checklist item #5718

Merged
merged 2 commits into from
Mar 14, 2022
Merged

Added a Figma specific PR checklist item #5718

merged 2 commits into from
Mar 14, 2022

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Mar 14, 2022

The UX org leads have been brainstorming ideas to try to keep the Figma library from falling out of relevance. We know that the EUI members can't be everywhere (and honestly we hardly use the Figma library ourselves), but one thing we can be more proactive on is ensuring that we consider the Figma side when we make visual updates to the components.

As per usual checklist tasks, you can just cross this one off if it doesn't apply to your PR.

We also know that this will more than likely fall on our designer team members so please be mindful of the extra workload/communication of design changes.

If it's not likely something to be tackled within the timeframe of the PR, there is now a Figma specific repo where we can add issues: https://github.com/elastic/eui-figma/issues

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

The best part of this PR 😄

Check in both light and dark modes

The new Figma checklist item looks good to me. I confirmed the link is the right one. 👍🏽

Copy link
Contributor

@breehall breehall 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!

@cee-chen
Copy link
Contributor

cee-chen commented Mar 14, 2022

No objections, but any chance we can get a quick TL;DR or rundown of what developers (say, ones who have never opened Figma before 🙈) should be doing for this checklist? Should we be attempting to L2Figma or should we be tagging Caroline or Elizabet in to handle that checkbox?...

Also does EuiDataGrid have a figma component? Asking for a friend (it's me, I'm my own friend)

edit: I leveled up by actually opening the figma link (fun fact, did you know firefox reports figma as a script slowing down the entire page) and saw that yes, yes we do have a data grid figma component hahaa

@cchaos
Copy link
Contributor Author

cchaos commented Mar 14, 2022

Lol. Yes @constancecchen the Figma community page is SUPER slow because for some reason Figma is trying to render all the pages in that box instead of just the one selected, and well, because our library is HUGE.

It's also going to be a learning curve for our productivity workflow. No, I don't really expect engineers to be updating the Figma side, but certainly to be considering the necessity of and figuring out who might be best to pair with to implement that side. For instance, if the originating issue came from a designer or other team, it might be asking for support from that designer or a designer from that team to update Figma side.

If those are unknown, then tagging either @miukimiu or myself is a fair approach. I just want to make sure we're mindful and not just crossing it off every time because it was an engineer's PR.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5718/

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Thanks a ton for the clarification!

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding it (+ updating the theme check)! ❤️

@cchaos
Copy link
Contributor Author

cchaos commented Mar 14, 2022

Thank you all for approving! We can also continue discuss this some more in our weekly on Tuesday.

@cchaos cchaos merged commit 6fe7965 into main Mar 14, 2022
@cchaos cchaos deleted the cchaos-patch-1 branch March 14, 2022 20:35
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.

8 participants