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

Notification component #319

Merged
merged 6 commits into from
Jul 28, 2023
Merged

Notification component #319

merged 6 commits into from
Jul 28, 2023

Conversation

Algorush
Copy link
Collaborator

I made css a separate file and imported inside viewer-style.css. I think it's more convenient or no?

@netlify
Copy link

netlify bot commented Jul 22, 2023

Deploy Preview for 3dstreet-core-builds ready!

Name Link
🔨 Latest commit dfcf91a
🔍 Latest deploy log https://app.netlify.com/sites/3dstreet-core-builds/deploys/64c32d173b45930008ee7804
😎 Deploy Preview https://deploy-preview-319--3dstreet-core-builds.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.

@Algorush
Copy link
Collaborator Author

It looks like I created this branch not from the master by mistake... I hope everything will be fine

@kfarr
Copy link
Collaborator

kfarr commented Jul 26, 2023

this is a great start, I tested using the following js console command:
AFRAME.scenes[0].setAttribute('notify','successMsg','This is super awesome, thanks Alex.')

some requests for updates, hopefully this isn't overthinking but makes it simpler for users:

  • i do like the idea of importing the css for this, thanks!
  • a few commented out lines index.html could be deleted
  • suggest to add 1 additional custom type 'info', color blue with no icon
  • therefore there are now 3 total types: 'error', 'success', 'info'; default should be 'info'
  • But, instead of 3 methods for sending a message, have 1 message method, for example it would be called like this:
    AFRAME.scenes[0].components['notify'].message('yoyoyo')
  • and then there is optional second variable in message method that is type ('error', 'success', 'info') so this would be an error message: AFRAME.scenes[0].components['notify'].message('something broke', 'error')
  • same feedback for the schema, there should be a new property 'type' that is default 'info' and can be one of 3 values 'error', 'success', 'info'; then there is only 1 property 'message' that triggers message sending; then remove the 'successMsg' and 'errorMsg' properties
  • default x value should be center instead of right

@Algorush
Copy link
Collaborator Author

Algorush commented Jul 27, 2023

I did everything as you suggested. It seems that all works good

@kfarr kfarr merged commit 9e7ef92 into main Jul 28, 2023
2 checks passed
@kfarr kfarr deleted the notification-component branch July 28, 2023 02:51
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.

2 participants