-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix(rtl-customisation): changed the header back button orientations o… #5984
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.
Hey @ApsMJ23 thank you for the contribution!
I've noticed that even though the changes are made in @refinedev/ui-types
for crud components and layout components; you've only updated the related parts of @refinedev/antd
.
We can only accept such changes if they are persistent on all UI implementations (ones using @refinedev/ui-types
)
But about your implementation, I think the RTL support can be done without any intervention from Refine from the top.
About the back button in <PageHeader />
, we can try to match direction
prop from <ConfigProvider />
of antd
since it will be the one responsible with rendering components to desired direction.
We can access the ConfigProvider
's context and use the direction
value inside PageHeader
to decide which icon to render.
Example below:
import React from "react";
import { ConfigProvider } from "antd";
const Component = () => {
const { direction } = React.useContext(ConfigProvider.ConfigContext);
return <Button icon={direction === "rtl" ? (...) : (...)} />
}
Got it @aliemir I'll make the necessary changes and get back to you. |
@aliemir Thanks for the help, I had no idea about the antd ConfigProvider, could you please take a look at this PR, and let me know if anything needs to be changed!! |
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.
Hey @ApsMJ23 great work! Just checked the PR and looks good to me! 👏
Can you add a changeset for the changes? Check out the Contributing Guide to learn how to do so.
Btw, can you try to add cases for the rtl
in the tests? Let me know if you can work on this 🙏
Also please let me know if I can help with the tests 🚀 🚀
2f00d07
to
16dc093
Compare
@aliemir sure I can work on the tests as well, Do I do this in this PR only or do I need to raise a different one? |
Hey @ApsMJ23 you can do it in this PR. |
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.
Hey @ApsMJ23, after the last month's release this branch got some issues and conflicts that needed to be resolved manually. I resolved them in my local and had to force-push.
Also added some tests and a changeset about the changes. Can you check if there's anything missing?
If not, we can schedule this PR to be included in our next release, thank you for your contribution! 🚀
@aliemir This looks good to me, It was a great learning experience I learned more by contributing on this than my day job xD, Please lemme know if I can do anything more to make refine better!! |
…f all the components and sidebar collapse button from left to right
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
In the current behaviour whenever we use the rtl-customisation example the arrows are not aligned from right to left
What is the new behavior?
The arrows are not aligned to the right to left format
fixes # (issue)
#5953 (comment)
Notes for reviewers
If anyone could help me with the documentation and changesheet part, I have never done that and I followed the documentation but I have some doubts