fix(*): Style updates, add edit issue body functionality #409

housseindjirdeh · 2017-10-02T02:26:02Z

Various fixes/updates for #381

Changes color of mark all notifications button

Adds padding for no notifications message in notifications screen

Fixes this problem:

Allows editing of issue description

Makes title/subtitle of IssueDescriptionListItem better in terms of boldness

Removes the user title from repository list item (@lex111 - with this it's unlikely we'll see the private box being squeezed anymore as most repo titles aren't nearly as long)

housseindjirdeh · 2017-10-02T02:29:26Z

src/issue/issue.action.js

@@ -172,6 +173,28 @@ export const editIssueComment = (issueCommentId, owner, repoName, body) => {
  };
 };

+export const editIssueBody = (owner, repoName, issueNum, body) => {


editIssue currently already exists and I'm definitely not a fan of how I'm passing updateParams to it. Because the comment is HTML, can't leverage that to pass in a new update parameter for the text to replace state.

Will need to refactor how editIssue works later without a doubt and instead of passing in updateParams, just get what's returned from the API (and we can then just remove this)

lex111 · 2017-10-02T04:45:51Z

Removes the user title from repository list item (@lex111 - with this it's unlikely we'll see the private box being squeezed anymore as most repo titles aren't nearly as long)

Let's remove this label and display this icon?

lex111 · 2017-10-02T04:52:59Z

Adds padding for no notifications message in notifications screen

And why are they needed? There in fact there is an alignment on the center, and after addition padding, any more will not be absolutely on the center to be displayed.

chinesedfan · 2017-10-02T06:38:11Z

@housseindjirdeh User titles are needed, at least for searched result or stared repositories. This feature is added by #254 in particular. And I also mentioned the redundancy, but you missed it. :)

machour · 2017-10-02T09:44:05Z

Seconding @lex111, let's use that emoji and bring back username/reponame :

If I'm on some user profile, then check his repos, I'll have to remember the user name, or navigate back and forth to have the repo full name to clone it

lex111 · 2017-10-02T09:52:18Z

src/components/repository-list-item.component.js

@@ -63,7 +63,7 @@ const renderTitle = repository =>
    <View style={styles.repositoryContainer}>
      <View style={styles.titleWrapper}>
        <Text style={styles.title}>
-          {repository.full_name}
+          {repository.name}


As correctly noted, displaying only the name of the repository will not be convenient, so we need to add a check: if this is an auth screen, then show only the name of the repository, and if the user profile, then display the full name.

Although, maybe it can just add the lock icon (if private) to the end of the title?
@chinesedfan so in fact will work?

Yes, it doesn't matter the pending element is the private box or the lock icon, as long as the parent element is Text and the child view has width&height specified.

jouderianjr · 2017-10-02T12:59:18Z

@housseindjirdeh What do you think about handling with edit issue body in another screen. Maybe we could create an edit issue for edit the description and the title.

housseindjirdeh · 2017-10-02T15:23:43Z

And why are they needed? There in fact there is an alignment on the center, and after addition padding, any more will not be absolutely on the center to be displayed.

Honestly I just felt that the message looks a bit squeezed for English copy?

Probably just imagining things, reverted it back and removed that padding :)

User titles are needed, at least for searched result or stared repositories. This feature is added by #254 in particular. And I also mentioned the redundancy, but you missed it. :)

Have no idea how I missed that point. Thank you, and I completely forgot that search and starred repos would have to show that. Updated it so now only the authenticated list of users repositories won't show (otherwise it will :))

Let's remove this label and display this icon?

Done:

housseindjirdeh · 2017-10-02T15:27:58Z

@jouderianjr so you make a good point, and right now if you head to the settings screen you can modify a few issue settings (but not title or description)

We could have issue title and description modifiable here but honestly I actually prefer at least the description being modified the same way as any other comment. It feels intuitive as I still think of it as a comment.

Definitely curious if you think there's a better option however, as well as how we can add functionality to edit the issue title in the future 🤔

lex111 · 2017-10-02T15:44:21Z

Honestly I just felt that the message looks a bit squeezed for English copy?

Oops, did not notice if padding left and right, then this should not affect the alignment, although at first glance it seemed to me that this is so. Excuse me, let's it return, but can we make them smaller?

lex111 · 2017-10-02T15:45:16Z

Let's remove this label and display this icon?

Do not you think that the size is big? In my opinion it was better if you reduce the size of the icon?

housseindjirdeh · 2017-10-02T15:52:40Z

Oops, did not notice if padding left and right, then this should not affect the alignment, although at first glance it seemed to me that this is so. Excuse me, let's it return, but can we make them smaller?

Cheers no worries. Added some padding but reduced it a bit :)

Do not you think that the size is big? In my opinion it was better if you reduce the size of the icon?

And nice you're probably right. Reduced it:

machour · 2017-10-02T15:52:58Z

@housseindjirdeh I'd rather use type="default" (or no type) for the "Mark all as read" button.
Let's keep "success" for strong actions, like merge

housseindjirdeh · 2017-10-02T15:58:09Z

@housseindjirdeh I'd rather use type="default" (or no type) for the "Mark all as read" button.
Let's keep "success" for strong actions, like merge

Done.

Although the default button looks interesting 🤔 Are these the intended colors @machour? (just showing it defaulted for the merge button for demo, don't have any notifications in the moment that's why :))

machour · 2017-10-02T15:59:18Z

@housseindjirdeh that icon should be black, otherwise it's the intended look.

jouderianjr · 2017-10-02T18:12:24Z

We could have issue title and description modifiable here but honestly I actually prefer at least the description being modified the same way as any other comment. It feels intuitive as I still think of it as a comment.

Definitely curious if you think there's a better option however, as well as how we can add functionality to edit the issue title in the future 🤔

I got it, make sense, so we can release that in v1.3 and then reorganize this screen in order to fit edit title and body 😄

Nice job mate!

housseindjirdeh · 2017-10-02T20:49:16Z

src/components/button.component.js

@@ -188,7 +191,12 @@ export class Button extends Component {
        }}
        disabledStyle={disabledStyle}
        icon={
-          icon && { ...defaultIconStyle, ...sizes[size].iconStyle, ...icon }
+          icon && {
+            ...defaultIconStyle,


Extended this since didn't seem like icons were being styled in the component within @machour

housseindjirdeh · 2017-10-02T20:50:46Z

@housseindjirdeh that icon should be black, otherwise it's the intended look.

@machour fixed:

housseindjirdeh · 2017-10-02T20:51:01Z

@jouderianjr Exactly what I was thinking :)

housseindjirdeh commented Oct 2, 2017

View reviewed changes

chinesedfan mentioned this pull request Oct 2, 2017

feat(issue): Show issues author and comments count #387

Merged

lex111 mentioned this pull request Oct 2, 2017

v1.3.0 Prep #381

Closed

lex111 reviewed Oct 2, 2017

View reviewed changes

housseindjirdeh mentioned this pull request Oct 2, 2017

fix(repo): Show repo prefix for search repo result #254

Merged

1 task

Houssein Djirdeh added 4 commits October 2, 2017 16:46

fix(*): Style updates, add edit issue body functionality

39110ea

refactor(*) Show lock icon for private repos + minor style updates

e217e30

style(*) Add padding for noneTitle + make private icon lock smaller

b04e24d

style(button) Add default icon style color

ed1d828

housseindjirdeh force-pushed the various-updates branch from 97812ec to ed1d828 Compare October 2, 2017 20:46

housseindjirdeh commented Oct 2, 2017

View reviewed changes

lex111 approved these changes Oct 2, 2017

View reviewed changes

housseindjirdeh merged commit 7e184e6 into gitpoint:master Oct 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(*): Style updates, add edit issue body functionality #409

fix(*): Style updates, add edit issue body functionality #409

housseindjirdeh commented Oct 2, 2017 •

edited

housseindjirdeh Oct 2, 2017

lex111 commented Oct 2, 2017

lex111 commented Oct 2, 2017

chinesedfan commented Oct 2, 2017

machour commented Oct 2, 2017

lex111 Oct 2, 2017

lex111 Oct 2, 2017 •

edited

chinesedfan Oct 2, 2017

housseindjirdeh Oct 2, 2017

jouderianjr commented Oct 2, 2017

housseindjirdeh commented Oct 2, 2017

housseindjirdeh commented Oct 2, 2017

lex111 commented Oct 2, 2017

lex111 commented Oct 2, 2017

housseindjirdeh commented Oct 2, 2017

machour commented Oct 2, 2017

housseindjirdeh commented Oct 2, 2017 •

edited

machour commented Oct 2, 2017

jouderianjr commented Oct 2, 2017

housseindjirdeh Oct 2, 2017

housseindjirdeh commented Oct 2, 2017

housseindjirdeh commented Oct 2, 2017

fix(*): Style updates, add edit issue body functionality #409

fix(*): Style updates, add edit issue body functionality #409

Conversation

housseindjirdeh commented Oct 2, 2017 • edited

housseindjirdeh Oct 2, 2017

Choose a reason for hiding this comment

lex111 commented Oct 2, 2017

lex111 commented Oct 2, 2017

chinesedfan commented Oct 2, 2017

machour commented Oct 2, 2017

lex111 Oct 2, 2017

Choose a reason for hiding this comment

lex111 Oct 2, 2017 • edited

Choose a reason for hiding this comment

chinesedfan Oct 2, 2017

Choose a reason for hiding this comment

housseindjirdeh Oct 2, 2017

Choose a reason for hiding this comment

jouderianjr commented Oct 2, 2017

housseindjirdeh commented Oct 2, 2017

housseindjirdeh commented Oct 2, 2017

lex111 commented Oct 2, 2017

lex111 commented Oct 2, 2017

housseindjirdeh commented Oct 2, 2017

machour commented Oct 2, 2017

housseindjirdeh commented Oct 2, 2017 • edited

machour commented Oct 2, 2017

jouderianjr commented Oct 2, 2017

housseindjirdeh Oct 2, 2017

Choose a reason for hiding this comment

housseindjirdeh commented Oct 2, 2017

housseindjirdeh commented Oct 2, 2017

housseindjirdeh commented Oct 2, 2017 •

edited

lex111 Oct 2, 2017 •

edited

housseindjirdeh commented Oct 2, 2017 •

edited