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: remove support links #2185

Closed
wants to merge 6 commits into from
Closed

Conversation

MrMartinR
Copy link
Contributor

@MrMartinR MrMartinR commented Jun 27, 2022

Fix for #2184

@parse-github-assistant
Copy link

parse-github-assistant bot commented Jun 27, 2022

Thanks for opening this pull request!

  • ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.

@MrMartinR MrMartinR changed the title 💄 Remove Support Links 💄 refactor: Remove Support Links Jun 27, 2022
@MrMartinR
Copy link
Contributor Author

Hi Guys (@mtrezza, @Moumouls, @dblythy, @cbaker6, @davimacedo, @dplewis) I think I am going to give up on this.
I removed the footer with the links, the mpn install showed some critical and high security stuff, I spent 1 hour trying to make the repo run, also because a bug in the passport dependency 0.6.0 (I had to install the 0.5.0)
I have no idea how to do tests and is something that I really do not want to learn right now (or maybe never 🤣🤔)
This PR supposed to "fix" this issue #2184
If someone has the system set up and know how to pass the workflow requirements, and want to fix this, it will takes 2 minutes to delete the footer.

@mtrezza
Copy link
Member

mtrezza commented Jun 28, 2022

@MrMartinR I don't think the passport dep has anything to do with this PR, simply removing the links like what you've done in this PR should be enough. I suggest you check your local node version, and if you cannot get dashboard to run locally, you could ask for help in chat.

Also:

  • Please always keep and use the template in issues and PRs.
  • If you need support to make a PR, please state clearly where you are stuck so others can understand and help.
  • If you need to bring attention to your PR, you could use the chat or tag the repo specific group, here @parse-community/dashboard rather than mentioning random users.
  • As you can see in the CI window, the commit message / PR title test fails; if you click on the link next to it, you can read a description for how a PR title needs to be formatted. I changed it already.

@mtrezza mtrezza changed the title 💄 refactor: Remove Support Links feat: remove support links Jun 28, 2022
@MrMartinR
Copy link
Contributor Author

Hi @parse-community/dashboard !, all the checks passed, can someone review this?

@MrMartinR MrMartinR requested a review from mtrezza July 3, 2022 14:10
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

If I'm not mistaken, I don't think there is a confirmation dialog before logging out. Maybe it's best for UX to keep the logout menu as a submenu, so it requires 2 clicks. That will also make it easier to add more menu options in the future.

And I'd remove that hand emoji. It's a style that isn't used anywhere else in the dashboard.

@MrMartinR
Copy link
Contributor Author

Yeah, I am not very happy with the emoji neither..
so, what do you propose? 3 dots button with the Log Out sub-menu?

@mtrezza
Copy link
Member

mtrezza commented Jul 3, 2022

Yes, maybe keep the 3 dots and only leave the Logout without the hand emoji? There are some icons in the dashboard repo where you may find a log out icon, but you don't really need to dig into that, not even sure if there is one.

@MrMartinR
Copy link
Contributor Author

There are no logout/exit icon, probably in the future I will come back to this and I will create a logout icon, I am not 100% happy with the css I had to tweak because is just 1 element in the popover, probably in the future when I am more familiar with the code I will improve that.

@MrMartinR MrMartinR requested a review from mtrezza July 4, 2022 00:18
@mtrezza
Copy link
Member

mtrezza commented Jul 4, 2022

Could you test out #2203 and see if it works? I've made a few changes to make the button larger when used with a touch UI and changes the menu text.

I actually opened the PR because this PR did not display the pop-up correctly when I was testing it out, so I thought you needed some help, but it tuns out that your button works fine. So you can either incorporate the changes in this PR and I'll close the other one, or you can close this one and I'll merge the other one, whatever you prefer. If you want to use this PR, please look for unnecessary changes like added empty lines or spaces, these should be removed.

@mtrezza mtrezza closed this Jul 4, 2022
@mtrezza mtrezza reopened this Jul 4, 2022
@MrMartinR MrMartinR closed this Jul 4, 2022
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