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

[Enhancement] Footer Changes #915

Merged
merged 8 commits into from
Apr 11, 2024
Merged

Conversation

DysektAI
Copy link
Contributor

@DysektAI DysektAI commented Apr 6, 2024

Edited the footer layout

Description πŸ—’οΈ

Changes
-Removed "Escape from Tarkov Dev tracker" seems inactive and rarely necessary unless this is required I don't see a use for it
-Removed the commented out link to "API Users" page
+Added "Tarkov Monitor" as a resource linked to the github since its not listed anywhere
~Re-ordered the Resources and External Sources to be more stylish

(Reverted)
Discord: Simplified to just use a shields.io img/button that shows users online count (Still requires widget to be enabled on server)
X/Twitter: Also changed to use shields.io img/button to view X/Twitter
Removed the " : " in the "Made with love by"

Examples πŸ“Έ

Before
image
After
image

Related Issues πŸ”—

#914 (Does not fix the issue but its planned)

@DysektAI DysektAI requested a review from a team as a code owner April 6, 2024 08:15
@GrantBirki
Copy link
Member

.deploy to development

Copy link
Contributor

github-actions bot commented Apr 6, 2024

⚠️ Cannot claim deployment lock

Sorry GrantBirki, the development environment deployment lock is currently claimed by Razzmatazzz

Lock Details πŸ”’

  • Environment: development
  • Branch: loadout-test
  • Created At: 2024-04-01T11:17:27.169Z
  • Created By: Razzmatazzz
  • Sticky: true
  • Global: false
  • Comment Link: click here
  • Lock Link: click here

The current lock has been active for 5d:6h:26m:20s

If you need to release the lock, please comment .unlock development

@Razzmatazzz
Copy link
Member

.unlock development

Copy link
Contributor

github-actions bot commented Apr 6, 2024

πŸ”“ Deployment Lock Removed

The development deployment lock has been successfully removed

Copy link
Contributor

@Shebuka Shebuka left a comment

Choose a reason for hiding this comment

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

Discord: Simplified to just use a shields.io img/button that shows users online count (Still requires widget to be enabled on server)
X/Twitter: Also changed to use shields.io img/button to view X/Twitter

These are some significant changes and I would like to keep them as they were for now, revert please.

Removed the " : " in the "Made with love by"

The use of a colon here is justified as it introduces a list of contributors, so please revert it back too :)

@DysektAI
Copy link
Contributor Author

DysektAI commented Apr 6, 2024

Discord: Simplified to just use a shields.io img/button that shows users online count (Still requires widget to be enabled on server)
X/Twitter: Also changed to use shields.io img/button to view X/Twitter

These are some significant changes and I would like to keep them as they were for now, revert please.

Removed the " : " in the "Made with love by"

The use of a colon here is justified as it introduces a list of contributors, so please revert it back too :)

Reverted now back to original formatting to comply with your request πŸ‘

@DysektAI DysektAI requested a review from Shebuka April 7, 2024 00:18
@Shebuka
Copy link
Contributor

Shebuka commented Apr 8, 2024

Discord: Simplified to just use a shields.io img/button that shows users online count (Still requires widget to be enabled on server)
X/Twitter: Also changed to use shields.io img/button to view X/Twitter

These are some significant changes and I would like to keep them as they were for now, revert please.

Removed the " : " in the "Made with love by"

The use of a colon here is justified as it introduces a list of contributors, so please revert it back too :)

Reverted now back to original formatting to comply with your request πŸ‘

I just looked at the changes and the discord iframe is still missing

@DysektAI
Copy link
Contributor Author

DysektAI commented Apr 8, 2024

I'm confused... the reason for this PR mainly was to fix the issue originally opened #914 unless maybe you know a way to fix the CORS error without removing the iFrame / making sure no one has a default profile picture / avatar and finding a way to fix the discord widget from extending off the page, I am all for it but currently there is no known resolutions hence why I said in the OP above:
(I did search for others who had this issue but it seems like the widget for discord was made in 2016 and the most recent posts I found were in 2022 with no answers that are similar to this)

Not sure why the default profile avatars for discord are throwing CORS errors so I assume it's best to just remove it / replace it (Also when hovered it extended past the bottom of the screen)

image

@DysektAI
Copy link
Contributor Author

DysektAI commented Apr 8, 2024

There is also already multiple references to the discord I attempted to use the shields.io as a replacement for the "online users" as that is nice to see but you asked to revert / remove that change.

@Shebuka
Copy link
Contributor

Shebuka commented Apr 8, 2024

There is also already multiple references to the discord I attempted to use the shields.io as a replacement for the "online users" as that is nice to see but you asked to revert / remove that change.

You replaced the iframe with the image in the PR, but using iframe is the current and only official way to embed a discord widget in the site.

Also, it does work and everything is loading fine so I want to keep using it.

@Shebuka
Copy link
Contributor

Shebuka commented Apr 8, 2024

I'm confused... the reason for this PR mainly was to fix the issue originally opened #914 unless maybe you know a way to fix the CORS error without removing the iFrame / making sure no one has a default profile picture / avatar and finding a way to fix the discord widget from extending off the page, I am all for it but currently there is no known resolutions hence why I said in the OP above: (I did search for others who had this issue but it seems like the widget for discord was made in 2016 and the most recent posts I found were in 2022 with no answers that are similar to this)

Not sure why the default profile avatars for discord are throwing CORS errors so I assume it's best to just remove it / replace it (Also when hovered it extended past the bottom of the screen)

image

As of 15 October 2023 they are still actively supporting the widget: discord/discord-api-docs#6447
I sincerely do not understand why you think the widget is bad?

Its expanding and changing footer height with animation is not ideal, but that can be solved with layout updates, not by removing the widget.

@DysektAI
Copy link
Contributor Author

DysektAI commented Apr 8, 2024

I'm confused... the reason for this PR mainly was to fix the issue originally opened #914 unless maybe you know a way to fix the CORS error without removing the iFrame / making sure no one has a default profile picture / avatar and finding a way to fix the discord widget from extending off the page, I am all for it but currently there is no known resolutions hence why I said in the OP above: (I did search for others who had this issue but it seems like the widget for discord was made in 2016 and the most recent posts I found were in 2022 with no answers that are similar to this)

Not sure why the default profile avatars for discord are throwing CORS errors so I assume it's best to just remove it / replace it (Also when hovered it extended past the bottom of the screen)

image

As of 15 October 2023 they are still actively supporting the widget: discord/discord-api-docs#6447 I sincerely do not understand why you think the widget is bad?

Its expanding and changing footer height with animation is not ideal, but that can be solved with layout updates, not by removing the widget.

I can give a list of reasons why I don't think the discord widget is good

  1. It expands and causes weird issues where users have to scroll awkwardly through a list of users to even see or access the "join server" button and expands the footer to over half of the monitor on desktop.
  2. The discord icon does not link to the server, it links directly to discords website which users will most likely click on if they don't scroll.
  3. Do users need to see the voice channels and users in the discord on the page before clicking join server?

If you insist on keeping the discord widget I would almost be more excited to create a customized version of it than using the default widget layout with something like the one below

image

Just because something "works and everything is loading fine" does not mean its the best option.

If you want to keep everything as is that's ok just edit my PR or close it I'll work on a custom widget if it is a must have when I have time.

@Razzmatazzz
Copy link
Member

I agree the current Discord widget has some issues (especially on mobile). It might make sense to just have a discord image and join link. But I could be overlooking some of the usefulness of the current widget.

@Blightbuster
Copy link
Member

My 5 cents: While the current discord widget is the official one, the two negatives @lyricalcasanova outlined above heavily outway that. Id be onboard with either disabling the folding and make the click action redirect to our join link instead of discords website or just using shields.io at that point. Im using shields.io for many of my repos as well and only ever had good experiences with it so far.

@DysektAI DysektAI changed the title Updated footer and made it cleaner [Enhancement] Footer Changes Apr 10, 2024
@Shebuka
Copy link
Contributor

Shebuka commented Apr 10, 2024

I'm confused... the reason for this PR mainly was to fix the issue originally opened #914 unless maybe you know a way to fix the CORS error without removing the iFrame / making sure no one has a default profile picture / avatar and finding a way to fix the discord widget from extending off the page, I am all for it but currently there is no known resolutions hence why I said in the OP above: (I did search for others who had this issue but it seems like the widget for discord was made in 2016 and the most recent posts I found were in 2022 with no answers that are similar to this)

Not sure why the default profile avatars for discord are throwing CORS errors so I assume it's best to just remove it / replace it (Also when hovered it extended past the bottom of the screen)

As of 15 October 2023 they are still actively supporting the widget: discord/discord-api-docs#6447 I sincerely do not understand why you think the widget is bad?
Its expanding and changing footer height with animation is not ideal, but that can be solved with layout updates, not by removing the widget.

I can give a list of reasons why I don't think the discord widget is good

  1. It expands and causes weird issues where users have to scroll awkwardly through a list of users to even see or access the "join server" button and expands the footer to over half of the monitor on desktop.
  2. The discord icon does not link to the server, it links directly to discords website which users will most likely click on if they don't scroll.
  3. Do users need to see the voice channels and users in the discord on the page before clicking join server?

If you insist on keeping the discord widget I would almost be more excited to create a customized version of it than using the default widget layout with something like the one below

Just because something "works and everything is loading fine" does not mean its the best option.

If you want to keep everything as is that's ok just edit my PR or close it I'll work on a custom widget if it is a must have when I have time.

It's not that "Just because something "works and everything is loading fine" does not mean its the best option."
You are pointing out some valid critiques now, but they were not in your initial PR where the main reason for replacement are CORS errors which are IMO harmless as they are thrown only for default avatars and the rest works fine. But you were not providing an apples-to-apples replacement, that's why I've asked it to be reverted as losing functionality for some harmless errors is not a solution.

My first glance comparison was:

shields.io:
Pros:

  • Simple
  • Users count

Cons:

  • No call to action
  • Styling is not DIscord's
  • No list of online users

Discord widget:
Pros:

  • Users count
  • Styling distinctive to Discord

when expanded:

  • List of online users
  • Call to action
  • Voice channels (opinable)

Cons:

  • Animation on expansion (fixable)
  • Voice channels (opinable)
  • Call to action only visible when expanded

No call to action in your shields.io implementation is a major stopper for me (it's also why I didn't like shields.io for X). As for other pros missing, I can deal with it if for others it's ok.

@DysektAI
Copy link
Contributor Author

Previous Replies > > > > I'm confused... the reason for this PR mainly was to fix the issue originally opened #914 unless maybe you know a way to fix the CORS error without removing the iFrame / making sure no one has a default profile picture / avatar and finding a way to fix the discord widget from extending off the page, I am all for it but currently there is no known resolutions hence why I said in the OP above: (I did search for others who had this issue but it seems like the widget for discord was made in 2016 and the most recent posts I found were in 2022 with no answers that are similar to this) > > > > > Not sure why the default profile avatars for discord are throwing CORS errors so I assume it's best to just remove it / replace it (Also when hovered it extended past the bottom of the screen) > > > > > > > > > As of 15 October 2023 they are still actively supporting the widget: [discord/discord-api-docs#6447](https://github.com/discord/discord-api-docs/issues/6447) I sincerely do not understand why you think the widget is bad? > > > Its expanding and changing footer height with animation is not ideal, but that can be solved with layout updates, not by removing the widget. > > > > > > I can give a list of reasons why I don't think the discord widget is good > > > > 1. It expands and causes weird issues where users have to scroll awkwardly through a list of users to even see or access the "join server" button and expands the footer to over half of the monitor on desktop. > > 2. The discord icon does not link to the server, it links directly to discords website which users will most likely click on if they don't scroll. > > 3. Do users need to see the voice channels and users in the discord on the page before clicking join server? > > > > If you insist on keeping the discord widget I would almost be more excited to create a customized version of it than using the default widget layout with something like the one below > > Just because something "works and everything is loading fine" does not mean its the best option. > > If you want to keep everything as is that's ok just edit my PR or close it I'll work on a custom widget if it is a must have when I have time.

It's not that "Just because something "works and everything is loading fine" does not mean its the best option." You are pointing out some valid critiques now, but they were not in your initial PR where the main reason for replacement are CORS errors which are IMO harmless as they are thrown only for default avatars and the rest works fine. But you were not providing an apples-to-apples replacement, that's why I've asked it to be reverted as losing functionality for some harmless errors is not a solution.

My first glance comparison was:

shields.io: Pros:

  • Simple
  • Users count

Cons:

  • No call to action

I find the Discord 300 Online more of a call to action than a generic discord widget, that users get lost just trying to find the "join server" button because it overflows off the screen then users have to scroll down users list to the bottom battling the on hover transition.
Not to mention we have a true call to action already in the footer...
image

  • Styling is not DIscord's

Styling in Shields is subjective, I simply used the color pallet that I thought looks clean
if needed discord colors can be used for that also

  • No list of online users

Do you really need to see the online users list in the discord before you join?
I can't remember a single time I checked a user list to see if I wanted to join a discord...

Discord widget: Pros:

  • Users count

User count - Online Users is also in Shields.io as one of your pros above
image

  • Styling distinctive to Discord

See above (Unless you mean the widget itself)

when expanded:

  • List of online users

See above

  • Call to action

See above

  • Voice channels (opinable)

Same argument as the users list I have never worried about how many users are in the voice channels especially for a server centered around developing and tarkov news/updates
(This would be different if the server was centered around just gaming)

Cons:

  • Animation on expansion (fixable)
  • Voice channels (opinable)
  • Call to action only visible when expanded

No call to action in your shields.io implementation is a major stopper for me (it's also why I didn't like shields.io for X). As for other pros missing, I can deal with it if for others it's ok.

With all respect, @Shebuka it seems we can't come to a solid agreement. You do have the option to contribute your own code / changes that you would like to see if you feel strongly about anything. From the consensus so far, it seems like you may be the only one not okay with removing this generic widget
I would understand your argument if you created the code being removed, but it's simply a widget that can always be moved to somewhere else or added later if a better option is presented.

I'm tired of debating over 32 changes, at this point unless there is truly a valid argument to make I will leave it up to the maintainers to decide what they want to do with this PR either Close, Edit, or Merge.

@Shebuka
Copy link
Contributor

Shebuka commented Apr 10, 2024

Oh man, just wow... In the last comment, I've explained to you the reason behind my INITIAL request to rollback... I thought it was clear with using the past tense...

Not to mention we have a true call to action already in the footer...

Where is a call to action here? (your screen)
image

YOU said that

I'll work on a custom widget

and Im saying ok if it has a call to action and is stylized like DIscord as YOU showed in the screenshot.

image

Where does even

With all respect, @Shebuka it seems we can't come to a solid agreement.

come from???

@Razzmatazzz
Copy link
Member

Seems like this discussion is causing some frustration for multiple people. Let's not lose sight of the fact that we all have the common goal of making the website as good as it can be to be as useful to the community as possible. Yes, we will sometimes have differences of opinion on what makes the website the best, but we should be able to discuss those differences respectfully and reach a consensus on the best way to move forward.

My two cents is that the current discord widget causes some annoying behavior (particularly on mobile). The shields.io solution seems like a good substitute as it provides a recognizable join link plus the current user count. While it does lack the ability to see who is online, voice channels, etc., I don't view those features as being vital (especially when crammed into the website footer).

@DysektAI
Copy link
Contributor Author

Oh man, just wow... In the last comment, I've explained to you the reason behind my INITIAL request to rollback... I thought it was clear with using the past tense...

Not to mention we have a true call to action already in the footer...

Where is a call to action here? (your screen) image

YOU said that

I'll work on a custom widget

and Im saying ok if it has a call to action and is stylized like DIscord as YOU showed in the screenshot.

image

Where does even

With all respect, @Shebuka it seems we can't come to a solid agreement.

come from???


The call to action is the shields.io with a count of the online users, this causes curiosity from the users to wonder what is in the discord which makes them want to click it.

The requests to rollback was done as you requested with the exception of the discord widget as you did not mention the widget in your original request here:

Discord: Simplified to just use a shields.io img/button that shows users online count (Still requires widget to be enabled on server)
X/Twitter: Also changed to use shields.io img/button to view X/Twitter

These are some significant changes and I would like to keep them as they were for now, revert please.

Removed the " : " in the "Made with love by"

The use of a colon here is justified as it introduces a list of contributors, so please revert it back too :)


Maybe there was some misunderstanding from the above comment but I see no reference in this comment anywhere specifying the discord widget not being removed simply just asking "revert please." with my change points referenced about the shields.io for x/twitter and discord this could not be further from clearly communicating in the future please provide more details, examples, or code so it's more clear.

I did as you requested which is the current state of this PR, then you asked where the widget is?
I told you I could make one later on, if required or you can contribute also.
You then continued to argue that its not valid to remove the widget, so basically should I not contribute and revert all the changes?
What would make you happy?
I'm a bit confused, maybe if you propose some code or a demonstration of what your changes would look like then I would understand better I would love to see what your final product looks like and how it functions compared to what I proposed.

@DysektAI
Copy link
Contributor Author

Also please enlighten me maybe my understanding of a "Call To Action" is wrong I thought a call to action was something that provokes people to click or find interest in doing something like a button / link? Is the button that says "Discord 283 Online" with a link to the Discord Server not a Call To Action?

Maybe it has to be a big bright button that follows none of the websites color scheme and jumps off the page or says "CLICK TO JOIN THE DISCORD" but I'm not sure because "Call To Action" in website design is defined as: A prompt on a website that tells the user to take some specified action. A call to action is typically written as a command or action phrase, such as 'Sign Up' or 'Buy Now' and generally takes the form of a button or hyperlink.

@Shebuka
Copy link
Contributor

Shebuka commented Apr 10, 2024

Maybe there was some misunderstanding, let's reset the conversation.


On this point:

I see no reference in this comment anywhere specifying the discord widget not being removed simply just asking "revert please." with my change points referenced about the shields.io for x/twitter and discord this could not be further from clearly communicating in the future please provide more details, examples, or code so it's more clear.

Briefly:

  • In the PR you post in changes: Discord: Simplified ...
  • Looking at code changes the iframe widget and text are removed, image is inserted (with online users, a feature of the widget)
  • I ask to rollback the change

I hope this helps understand the whole conversation flow and clears the confusion.


About what is a "Call To Action":

As you quoted it is any form of 'visual' message (a text, a button, an arrow) that invites the user to do something.

An image with "Discord 283 Online" is a statement of fact, like "build: passing" (see below). By look and feel it has all the functions of an image, no user will think it's a button without hovering the mouse over it (can't be done on mobile), and even after that they don't know what it does (URL in the status bar is not telling much), so it's clearly not a "Call To Action".

So, like what you showed is a perfect component with a clear call to action:
image

p.s. I never saw use of shileds outside of github and other related coding websites, so to general Tarkov website users it's just an image, as it is the build-passing-brightgreen

@DysektAI
Copy link
Contributor Author

Note: Accidentally merged the wrong branch (#911) force pushed to revert the merge, I would normally just add a revert commit but this was necessary

@Shebuka and @Razzmatazzz
I misunderstood what was exactly wanted / expected, but I feel like I was misunderstood also.

  • My intentions were not to just fully remove the widget forever, I just did not want to rush another solution and make this PR even more difficult.
  • The goal was a simple edit + remove the widget to keep console logs clean (and close [Issue] Discord Widget - CORS ErrorsΒ #914) while working on a suitable replacement to be added in a later PR separating concerns.
  • When I sent the example of the customized widget, it was simply an image for validation / reference. I have not currently made a replacement, but checked the code examples to ensure it was possible. (This most likely seemed like I had a ready to go, drop in replacement due to the example) I realize now I didn't even explain this so I see the frustration here.
  • What Confused Me: The section in the footer reverted back: If you wanna have a chat, ask questions or request features, we have a Discord server. satisfies the need of having a stable reference / call to action without the widget to access the discord. (Hyperlink to the discord server)
  • Moving forward, if possible we should be a little bit more precise about what is wanted, expected, or planned to avoid anymore confusion or miscommunication so we can focus on developing meaningful changes and less on wasting time. (This goes for myself as well, since this is my 1st/2nd PR ever let alone working with others on this project specifically so I appreciate the patience but need the clarity)

I have reverted the change and included the Discord Widget until there's an acceptable replacement for it as Shebuka is right it's best to have both options until then.

@Razzmatazzz
Copy link
Member

.deploy to development

Copy link
Contributor

Deployment Triggered πŸš€

Razzmatazzz, started a branch deployment to development

You can watch the progress here πŸ”—

Branch: 2a93717c1e32f51bf301252493e33b56f122aba4

Copy link
Contributor

Deployment Results βœ…

Razzmatazzz successfully deployed branch 2a93717c1e32f51bf301252493e33b56f122aba4 to development

Show Results

https://44b24540.tarkov-dev.pages.dev

@Shebuka
Copy link
Contributor

Shebuka commented Apr 11, 2024

.deploy

Copy link
Contributor

Deployment Triggered πŸš€

Shebuka, started a branch deployment to production

You can watch the progress here πŸ”—

Branch: 6e42b4a2e58decf6ac8c55e7b1e28f5ba878a3ad

Copy link
Contributor

Deployment Results βœ…

Shebuka successfully deployed branch 6e42b4a2e58decf6ac8c55e7b1e28f5ba878a3ad to production

Environment URL: tarkov.dev

@Shebuka Shebuka merged commit e6fc4cd into the-hideout:main Apr 11, 2024
2 checks passed
@DysektAI DysektAI deleted the Discord-CORS branch April 11, 2024 09:35
@github-actions github-actions bot mentioned this pull request Apr 11, 2024
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

5 participants