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

chore(docs&&bufey-next):fix defined in MessageMixin.js. #124

Merged

Conversation

zhanghengxin
Copy link

@zhanghengxin zhanghengxin commented Oct 30, 2023

@netlify
Copy link

netlify bot commented Oct 30, 2023

Deploy Preview for buefy-nextdocs ready!

Name Link
🔨 Latest commit 8042c9c
🔍 Latest deploy log https://app.netlify.com/sites/buefy-nextdocs/deploys/6542244bc59d120008466127
😎 Deploy Preview https://deploy-preview-124--buefy-nextdocs.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@wesdevpro wesdevpro left a comment

Choose a reason for hiding this comment

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

LGTM✨ Please wait for @kikuomax to review this PR before pushing to MigrateToLerna

  • Component works exactly like https://buefy.org
  • Netlify Builds ⚙
  • Code looks clean ✌

@kikuomax
Copy link
Collaborator

kikuomax commented Oct 31, 2023

@zhanghengxin @wesdevpro This PR introduces an unexpected progress bar on every component using MessageMixin, e.g, Message, whenever autoClose is true even if progressBar is false. You can compare the behavior between
https://buefy.org/documentation/message#auto-close
and
https://deploy-preview-124--buefy-nextdocs.netlify.app/documentation/message#auto-close

@kikuomax
Copy link
Collaborator

@zhanghengxin @wesdevpro The item on the project board is related to buefy#3855

Copy link
Collaborator

@kikuomax kikuomax left a comment

Choose a reason for hiding this comment

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

@kikuomax kikuomax closed this Oct 31, 2023
@zhanghengxin
Copy link
Author

netlify

Since autoClose serves additional purpose, itscontrol socope should not be limited to progress part and should be elevated.

such as:
image

@kikuomax
Copy link
Collaborator

kikuomax commented Nov 1, 2023

@zhanghengxin Here is the context of how autoClose && progressBar condition was introduced. We cannot simply regress without addressing potential drawbacks.
buefy#3754

@kikuomax kikuomax reopened this Nov 1, 2023
@zhanghengxin
Copy link
Author

zhanghengxin commented Nov 1, 2023

@kikuomax

I am currently debugging locally and have some development-related questions. At fix/draft-form-migrateToLerna

First question: I am using the command "npx lerna run dev --scope=docs" to run the documentation and observe the behavior of the components. However, I noticed that when I make changes in the code, I am unable to see the updates in real-time in the docs project. Do I need to manually connect to the local component project to resolve this?

Second question: Is there a direct method to debug the component project? I noticed that when I directly run the "npm run dev" command in the buefy-next project, it requires many dependencies to be installed. Is buefy-next currently runnable without any additional steps?

I would appreciate some guidance on these matters.

@zhanghengxin
Copy link
Author

zhanghengxin commented Nov 1, 2023

@zhanghengxin Here is the context of how autoClose && progressBar condition was introduced. We cannot simply regress without addressing potential drawbacks.

And for the answer.
I have carefully studied his features and code, and I have been trying to understand their functionality. Here are my findings:

@kikuomax
Copy link
Collaborator

kikuomax commented Nov 2, 2023

@zhanghengxin

First question: I am using the command "npx lerna run dev --scope=docs" to run the documentation and observe the behavior of the components. However, I noticed that when I make changes in the code, I am unable to see the updates in real-time in the docs project. Do I need to manually connect to the local component project to resolve this?

Due to my recent commit (1b80898), you have to build packages/buefy-next before running packages/docs. I thought this was good for testing @ntohq/buefy-next package in a more realistic configuration.

Second question: Is there a direct method to debug the component project? I noticed that when I directly run the "npm run dev" command in the buefy-next project, it requires many dependencies to be installed. Is buefy-next currently runnable without any additional steps?

You need to run packages/docs to debug packages/buefy-next. You may run unit tests in packages/buefy-next, but I do not think this is what you want to do. Btw, unit tests won't pass unless you remove packages/buefy-next/node_modules folder for now.

@zhanghengxin
Copy link
Author

@kikuomax
OK, what's need i still?

@kikuomax
Copy link
Collaborator

kikuomax commented Nov 2, 2023

@zhanghengxin Thanks for your analysis.

I understood that his point was introduction of the auto-close-progress CSS class to preserve the styles of ordinary progress bars on Message and Notification, and autoClose condition did not really matter.

with auto-close-progress:
image

without auto-close-progress:
image

kikuomax

This comment was marked as outdated.

kikuomax

This comment was marked as outdated.

Copy link
Collaborator

@kikuomax kikuomax 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, but could you amend the last commit message? As "WIP" does not make sense.

@zhanghengxin Sorry, you do not have to do this. It should not matter as long as we squash merge your PR.

@zhanghengxin
Copy link
Author

Looks good to me, but could you amend the last commit message? As "WIP" does not make sense.对我来说看起来不错,但你能修改最后的提交消息吗?因为“WIP”没有意义。

@zhanghengxin Sorry, you do not have to do this. It should not matter as long as we squash merge your PR.

ooo,Is there anything else i need to do before squashing and merging ?

@kikuomax
Copy link
Collaborator

kikuomax commented Nov 3, 2023

ooo,Is there anything else i need to do before squashing and merging ?

@zhanghengxin We are good to go!

@kikuomax kikuomax merged commit 2a866a5 into ntohq:MigrateToLerna Nov 3, 2023
4 checks passed
kikuomax pushed a commit that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants