-
Notifications
You must be signed in to change notification settings - Fork 29
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
Studio: Add new modal design #1128
Conversation
@ivan-ottinger Feel free to give it an initial review while I am polishing a couple of things |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivan-ottinger Feel free to give it an initial review while I am polishing a couple of things
Very nice! It great to see the modal coming to life! 🕺🏼
Just sharing a couple smaller things I have noticed so far in the comments below.
I have also noticed the design has the Previous
button in a border, but I think we are generally now trying to use borderless secondary buttons. We can double-check with Marina.
Shouldn't it be an item in the top menu? Other feedback:
|
Yes, there will be: #1129. We have beeing using the sidebar temporarily in development. Kat had left a comment here yesterday: https://github.com/Automattic/studio/pull/1128/files#r2012860549 Also, I have still considered this PR as a work-in-progress, not ready for final review. |
Sure, we can consider adding it to work around the fact that Learn More and Previous might seem to close. |
Here's where we were recently removing border from secondary buttons in Calypso: Automattic/wp-calypso#101088. That was originally brought up in Staging sites walkthrough: pesfPa-1n1-p2. This is why I think it is the right approach to keep the secondary buttons without the border in Studio as well. |
The dots were actually there but I was fixing something else and accidentally changed their color. This was fixed in 7139638 |
Makes sense, should we get some feedback from Marina? Maybe we can add a light background or something? Not sure. I think the problem with no border is that it causes the alignment issue that @wojtekn mentioned in his feedback. |
Yes, I will ask her in that Linear issue. |
Let's double-check with Marina, I followed what we have in Figma which is 312px. cc @ivan-ottinger |
Maybe you meant too tall, @wojtekn? 🤔 Or do you mean that it would look better if it is a bit more narrow generally? |
My bad @ivan-ottinger , yes, I meant too narrow and too tall :- ) |
8e8ae64
to
84b3528
Compare
I have now made the following updates:
Remaining things:
|
Nice! Thank you for all the updates, Kat. What if we used the following description for the custom domains card? We can still change it later if we decide to.
I think we could also use the copy suggested by Wojtek above for the first intro card:
One small additional thing I noticed is that the |
I made some changes, feel free to take another look @ivan-ottinger . One thing that is tricky is that with translations, some content might be longer so I am slightly worried about scrollbars appearing. I am wondering if we might set max-height instead of fixed height |
@katinthehatsite for long content you could make the footer sticky, so users could scroll the content, but still be able to navigate. If you want to try it, you can do this in CSS:
|
Thanks for the exploration, I am trying to avoid using CSS and looking for Tailwind approach. I do have the CSS solution already which would be something like:
I would also like avoid the scrollbars on the side. We should probably cut the text there. |
{ | ||
image: previewSitesIllustration, // TODO: Add correct illustration | ||
title: __( 'Choose a custom domain for your Studio site' ), | ||
description: __( | ||
'Easily identify your local Studio sites with custom domain names. Personalize and organize your workflow!' | ||
), | ||
learnMoreUrl: | ||
'https://developer.wordpress.com/docs/developer-tools/studio/sites/#1-using-a-custom-domain ', // TODO: Add correct URL | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some TODOs here, should we address them before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No these are fine, we are waiting for design. We will create a follow up issue to handle these
Thank you for all those changes, Kat! To mitigate the need for scrollbars, I wonder if we could make the modal height depending on the longest card content. 🤔 Other approach coming to my mind is to make the modal a little bit larger in general to accommodate longer translations. The "What's New" modal in Calypso has this extra space that can be filled with the content without affecting the modal size or footer button positions: If the content gets larger than the buffer, in those cases it could just make the modal taller / or add a scrollbar. In regards the use of CSS instead of Tailwind for this component in Studio, I wonder what is the point of view of other Yolo members. One last comment: I think we could still merge the PR even if it is not 100% the way we want it since we are not yet launching to production. We could at least get the translations going and have the "What's New" modal ready for internal testing. It would also unblock the beta release. 🙂 |
I agree that we can merge this as a base and then discuss the specifics. Currently, it will work like this:
I think a good approach from here would be to review this with the designer and see what improvements they could suggest to accommodate the translations etc.
I think in 95% of the time we should use Tailwind. However, this component is very tricky to handle as it allows minimal customizations. We can revisit if we can find an alternative approach for this. |
Sounds good to me. 👍🏼 Thanks for sharing the details, Kat.
Yes, let's discuss this with Marina. We can schedule a call with her for the beginning of the week when we get back from meetup - or - maybe we will find some time next week. |
Related issues
STU-226
Proposed Changes
This PR adds a modal to Studio according to Figma: RToz6tIuQ7nlZrikBte4GU-fi-7169_205567
Testing Instructions
STUDIO_WHATS_NEW_SECTION=true npm start
Help > What's New
menu button in the topbarPre-merge Checklist