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

feat(components): add el-text component #11653

Merged
merged 7 commits into from
Mar 10, 2023
Merged

feat(components): add el-text component #11653

merged 7 commits into from
Mar 10, 2023

Conversation

gimjin
Copy link
Contributor

@gimjin gimjin commented Feb 19, 2023

Please make sure these boxes are checked before submitting your PR, thank you!

  • Make sure you follow contributing guide English | (中文 | Español | Français).
  • Make sure you are merging your commits to dev branch.
  • Add some descriptions and refer to relative issues for your PR.

Because vitepress import from published element-plus, so el-text doc example not working. The docs only be packaged after published element-plus has el-text.

Locally review, build first, and edit example vue files:

<template>
  <el-text class="mx-1" size="large">Large</el-text>
  <el-text class="mx-1">Default</el-text>
  <el-text class="mx-1" size="small">Small</el-text>
</template>

+ <script lang="ts" setup>
+ import { ElText } from '../../../dist/element-plus/es/index.mjs'
+ </script>

@pull-request-triage
Copy link

👋 @gimjin, seems like this is your first time contribution to element-plus.

  • Please make sure that you have read our guidelines and code of conduct before making a contribution.
  • You can comment with /label Components:[component_name] to add a label for which component you are working on.
  • You may join our Discord for staying tuned.

@pull-request-triage pull-request-triage bot added 1st contribution Their very first contribution Needs Review labels Feb 19, 2023
@github-actions
Copy link

github-actions bot commented Feb 19, 2023

@github-actions
Copy link

Hello @gimjin, thank you for contributing to element-plus, please see our guideline to see how to make contribution

@gimjin
Copy link
Contributor Author

gimjin commented Feb 19, 2023

Create el-text idea by How to use Element-Plus only, forget inefficiency DIV+CSS?

@jw-foss
Copy link
Member

jw-foss commented Feb 22, 2023

@gimjin thank you for the contribution, I will go through your PR and see if this matches our current design principle, we WILL ACCEPT this one eventually.

@github-actions github-actions bot added the CommitMessage::Qualified Qualified commit message label Feb 22, 2023
@jw-foss
Copy link
Member

jw-foss commented Feb 22, 2023

@gimjin I was wondering if you can reorganize your commits into smaller pieces with git reset --mixed, for instance:

  • Component implementation
  • Testing
  • Documentation

Dividing your changes into smaller ones will boost the review process.

@github-actions
Copy link

github-actions bot commented Feb 22, 2023

🧪 Playground Preview: https://element-plus.run/?pr=11653
Please comment the example via this playground if needed.

@jw-foss
Copy link
Member

jw-foss commented Feb 22, 2023

To make this work, you will need to introduce this component in packages/element-plus/component.ts file and include it in the default exports.

@gimjin
Copy link
Contributor Author

gimjin commented Feb 23, 2023

@gimjin thank you for the contribution, I will go through your PR and see if this matches our current design principle, we WILL ACCEPT this one eventually.

thanks for your guidance @jw-foss , I've adjusted as required.

  • reorganize commits into smaller pieces
  • el-text in packages/element-plus/component.ts file and include it in the default exports

@gimjin
Copy link
Contributor Author

gimjin commented Feb 25, 2023

Already modified code and pushed. @ryuhangyeong

@gimjin
Copy link
Contributor Author

gimjin commented Feb 26, 2023

Do I want to synchronize with upstream/dev every push? I have just rebased the latest code.

git rebase -i upstream/dev
git push -f

@jw-foss
Copy link
Member

jw-foss commented Feb 26, 2023

Do I want to synchronize with upstream/dev every push? I have just rebased the latest code.

git rebase -i upstream/dev
git push -f

Technically you don't. Your commits will be squashed and merged onto the HEAD, meaning that you will only have 1 commit in the end, as long as there were no conflicts I don't think you should rebase for every push. BTW, in practical Git, we don't really use -i for rebasing the upstream, and for pushing, it is always recommended to use --force-with-lease instead of --force/-f, considering this is your own feature branch, -f is fine, just for making sure you don't accidentally push something to a shared branch, you should do that for every force push.

@gimjin
Copy link
Contributor Author

gimjin commented Feb 27, 2023

#11724 PR is before this PR merged dev branch, github robot tip me behind and show MERGE BUTTON, after click the button robot auto create new merged dev commit. Because this is not my commit and pollute dev branch history, so run git force rebase.

I already know, only conflict when merging will rebase it.

@jw-foss thanks gift --force-with-lease unfamiliar option.

@gimjin gimjin requested a review from tolking March 2, 2023 07:06
@ryuhangyeong
Copy link
Member

@gimjin nice~

@gimjin
Copy link
Contributor Author

gimjin commented Mar 6, 2023

@gimjin nice~

😸🥂

@jw-foss
Copy link
Member

jw-foss commented Mar 7, 2023

@gimjin thank you for your hard working!! I will review the ASAP 👍

Copy link
Member

@jw-foss jw-foss left a comment

Choose a reason for hiding this comment

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

@gimjin good job and excellent work! I have no further comments but one for the API naming, would you mind address it so we can merge this one and get it released!

docs/en-US/component/text.md Show resolved Hide resolved
Copy link
Member

@jw-foss jw-foss left a comment

Choose a reason for hiding this comment

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

@gimjin good job and excellent work! I have no further comments but one for the API naming, would you mind address it so we can merge this one and get it released!

@jw-foss
Copy link
Member

jw-foss commented Mar 7, 2023

I will merge this later this week, due to the hotfix tonight.

@jw-foss jw-foss merged commit 4f78411 into element-plus:dev Mar 10, 2023
@jw-foss
Copy link
Member

jw-foss commented Mar 10, 2023

@gimjin thank you for your excellent work and patient ❤️, I just merged this PR! Good job! 🚀

@ryuhangyeong
Copy link
Member

@gimjin Thank you for your hard work~

@gimjin
Copy link
Contributor Author

gimjin commented Mar 10, 2023

Thank you for review my graffiti code @jw-foss @ryuhangyeong @tolking @btea 🍻.

I have developed ellipsis tooltip and line-clamp in my line-clamp branch, but unit test some exceptions in el-tooltip, I will create PR after fix.

@element-bot element-bot mentioned this pull request Mar 12, 2023
3 tasks
loosheng pushed a commit to loosheng/element-plus that referenced this pull request Mar 26, 2023
* test(components): [text] el-text unit test

* docs(components): [text] el-text website documentation

* feat(components): [text] el-text implementation

* fix(components): [text] prop 'as' rename 'tag'

* docs(components): [text] rename slot default, optimize document

* test(components): [text] render text & class change the execution order

* fix(components): [text] use template and render function together
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.

None yet

6 participants