-
Notifications
You must be signed in to change notification settings - Fork 55
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 side panel (without unrelated changes) #585
Conversation
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.
Thank you so much for having split the big PR in different ones, it makes a huge difference for me 🥺
Below are a few feedback:
You're missing a background-color on the panel ;)
I don't know what "heroicons" is, but here is how I managed icons in this repository:
- I put my svg in the
icons/svg
folder - I run
yarn icons
- It transforms the SVG into react components that I can use in my app
I'm not against using a different system, but I'd be in favor or consistency especially since this repo is an open-source one :)
I submitted a weird email address for the newletter (like [email protected]) and it got submitted successfully. I don't know how we plan to handle those but raising it just in case :)
I submitted the newsletter form successfully and wanted to fill it again, but even when I closed the panel and reopened it, I didn't have the input anymore, just the success message. Should we "reset" the state when we close the panel ?
The panel is opened by default. If I refresh my page, it opens. I'm afraid this behaviour could be annoying for our users. What would you think of opening it by default unless the user previously closed it? We could store a variable in the localStorage for that :)
Apart from those small things it's working well 🚀
Thanks a lot for your work here 🙌
} | ||
` | ||
|
||
const Input = styled.input` |
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.
Out of curiosity, why didn't you use the Input component and override its style if needed?
} | ||
` | ||
|
||
const Button = styled.button` |
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.
Same question as for the Input :)
Hey, thanks for the review. I fixed the errors. For the improvements, I would recommend doing that in an update if they're not essential. Especially since @curquiza wants to do the release today.
☑️ Indeed, the
I didn't see the point of keeping both the SVG and the components. So I directly saved the SVGs in components. (Heroicons is just an icon library, I named the folder like this so it's more self-documenting.)
Yes, we agreed to roll it out like this and update it later if needed.
I don't think it's an important user journey, so I would recommend doing that in an update.
I'm not sure that's what @meilisearch/product-team wants. But I think it would be a welcome improvement. |
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.
Approving since I don't want to block the release, but I suggest to keep track of the enhancements in an issue so that we can tackle them later :)
Thanks for the quick review! I create the following issues to track the possible enhancements: |
Hi @mdubus, apologies for the huge PR earlier.
This PR should be easier to review. It replaces #580.
Related issue
Fixes partially meilisearch/meilisearch#5361
What does this PR do?
Adds a side-panel on the right of the app which contains
Removes
Address the first round of feedback on #580
☑️ Removed all changes on the API key modal, so I restored the previous behavior.
☑️ Added the ability to scroll in the side panel.
☑️ The side panel now uses a close (cross) icon instead of a hamburger.
☑️ Created a Cypress command to store the API token to local storage.