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

Profile settings #1280

Merged
merged 18 commits into from
Dec 31, 2017
Merged

Profile settings #1280

merged 18 commits into from
Dec 31, 2017

Conversation

Sekhmet
Copy link
Contributor

@Sekhmet Sekhmet commented Dec 28, 2017

Fixes #1256

image

image

Changes:

  • Add UI.
  • Add translations.
  • Use antd form.
  • Generate SteemConnect link.
  • Link to ProfileSettings.
  • Load values into form.
  • Use only updated values.
  • Update UserInfo.

@bonustrack bonustrack temporarily deployed to busy-master-pr-1280 December 28, 2017 21:22 Inactive
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1280 December 28, 2017 23:23 Inactive
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1280 December 29, 2017 12:03 Inactive
@bonustrack
Copy link
Contributor

@Sekhmet

  • Can we have the button "Save" not full-width? We need the same one than settings page
  • I think social links should not include the website, i would see another group for it

@bonustrack
Copy link
Contributor

Also it may be good to allow only alphanumeric + -_. characters to avoid having user who type the full url of their social profile

@Sekhmet
Copy link
Contributor Author

Sekhmet commented Dec 29, 2017

So social links shouldn't be URLs but only usernames? How to handle it if users doesn't have username specified (e.g. Facebook)? What do we want to do with Bitcoin/Ethereum? Would it be just address? How do we display it?

@bonustrack
Copy link
Contributor

On Facebook if the username is not specified, the id should work.

@bonustrack
Copy link
Contributor

@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1280 December 30, 2017 14:13 Inactive
@Sekhmet
Copy link
Contributor Author

Sekhmet commented Dec 30, 2017

Updated, example profile there:
https://busy-master-pr-1280.herokuapp.com/@sekhmet

@bonustrack
Copy link
Contributor

I'm thinking that the official BitCoin icon come with the circle around like this:
image
Same thing goes for Facebook with the square:
image

Is it possible that you use these instead?

@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1280 December 30, 2017 16:12 Inactive
Copy link
Contributor

@bonustrack bonustrack left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jm90m jm90m left a comment

Choose a reason for hiding this comment

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

looked through.
I tested.
I made comments.
you can do whatever you like, they are just suggestions, I'm still sick right now in Japan, but ill try to keep thoroughly reviewing


SocialLink.propTypes = {
profile: PropTypes.shape({
id: PropTypes.string.isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we start defining the properties for certain shapes now for propTypes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we know shape ahead of time, and it doesn't require much work to define properties it might be useful.

import _ from 'lodash';
import SocialLink from './SocialLink';

export const socialProfiles = [
Copy link
Contributor

Choose a reason for hiding this comment

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

might be good a idea to keep socialProfiles, socialTransformers and transform in a helper / constants file instead of a components file

import SocialLink from './SocialLink';

export const socialProfiles = [
{ id: 'facebook', icon: 'facebook', color: '#3b5998', name: 'Facebook' },
Copy link
Contributor

Choose a reason for hiding this comment

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

So I noticed we have a lot of colors floating around in our codebase in our javascript files...
would be nice to be keep them in a constants/styles.js file and import it based on below see jist here:

https://gist.github.com/jm90m/933417aaf8f1259804957be4df090a91

so import { COLORS } then you get the color from there, i figure out the color name by using this website http://www.color-blindness.com/color-name-hue/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we have many colour definitions in our JS files. Those are only useful in context of social profiles.
Things is we already have file with colours (less) and I don't think we can combine those two.

@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1280 December 31, 2017 10:25 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants