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: Create sharing menu #300

Merged
merged 14 commits into from
Aug 31, 2023

Conversation

john0isaac
Copy link
Contributor

@john0isaac john0isaac commented Aug 21, 2023

closes #255

  • Import Material Menu Library.
  • Replaced sharing button with sharing Menu.
  • Added a string parameter to track the clicked button.
  • Redefined Sharing Function.
  • Included if statements to check the clicked button and configure sharing accordingly.

While working on this I found a new issue the hashtags weren't added correctly to the twitter sharing link.
I fixed this issue now if you try to share on twitter the hashtags will appear.

Adding more platforms now will be much easier as it's only a copy-and-paste process. (see this)
image

@john0isaac
Copy link
Contributor Author

  • Added Spaces around imports, if statement.
  • Added check for the platform value definition.
  • Destructure the code for sharing URL, URL of the page.

I couldn't figure out the part where you mentioned creating a global object for the sharing URL I did it inside the function as I didn't know if there was a file to create the global variables in or if should I just have included them after the imports in the same file.
I did create an object though inside the share function.

As for the part where you recommend using the ternary operator I think it will get more complicated as we add more social media platforms so, I wouldn't recommend it although it's a beautiful idea if we only had two platforms.

Thanks again for the amazing feedback.

sinedied
sinedied previously approved these changes Aug 29, 2023
Copy link
Contributor

@sinedied sinedied left a comment

Choose a reason for hiding this comment

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

Just added a small suggestion, as Twitter has been renamed to X we might as well do so here 😉

@john0isaac
Copy link
Contributor Author

@sinedied I approved your changes.
Thank you for reviewing!
You can merge now.

Copy link
Member

@anfibiacreativa anfibiacreativa left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@anfibiacreativa anfibiacreativa added Hack-together:accepted Accepted after triage and ready for to be done and removed Hack-together:awaiting-feedback Awaiting feedback from participant labels Aug 31, 2023
Copy link
Member

@anfibiacreativa anfibiacreativa left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@anfibiacreativa anfibiacreativa merged commit dc778bf into Azure-Samples:main Aug 31, 2023
@john0isaac john0isaac deleted the create-sharing-menu branch August 31, 2023 16:17
devin-ai-integration bot pushed a commit to altsang/contoso-real-estate that referenced this pull request May 14, 2024
* Import material menu

* Replace share button with menu

* link three menu items to share function

* feat: setup share to different platforms

* fix: twitter hashtags

* feat: configure facebook sharing

* style: add spaces around  imports

* style: add spaces around if

* feat: check platform exists

* refactor: destructure sharing url varianle

* Update packages/portal/src/app/rentalpage/rentalpage.component.html

Co-authored-by: Yohan Lasorsa <noda@free.fr>

---------

Co-authored-by: Yohan Lasorsa <noda@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hack-together:accepted Accepted after triage and ready for to be done Hack-together
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HackTogether: Share Property to More social media
3 participants