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.

Sign up for GitHub

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

Jump to bottom

Added tags on list view #3077

Merged
merged 4 commits into from May 31, 2017
Merged

Added tags on list view #3077

merged 4 commits into from May 31, 2017

Conversation

nicosomb
Copy link
Member

@nicosomb nicosomb commented May 3, 2017 •
edited

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Documentation no
Translation no
Fixed tickets #2808, #2867
License MIT

capture d ecran 2017-05-29 a 15 21 31

@nicosomb nicosomb added this to the 2.3.0 milestone May 3, 2017
@tcitworld tcitworld mentioned this pull request May 4, 2017
Merged
10 tasks
@j0k3r j0k3r added the UI label May 9, 2017
j0k3r
j0k3r reviewed May 9, 2017
View reviewed changes
Copy link
Member

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

Looks good, we just need to fix the UI.
Should we fix it in that PR or open a new issue?
Any idea on how to fix the glitch with long tag?

@tcitworld
Copy link
Member

Will do tonight.

@tcitworld
Copy link
Member

Seems I forgot.

@nicosomb
Copy link
Member Author

Comment on twitter:

Perhaps with a differentiation between title and urls colors and less padding around tags.

@nicosomb
Copy link
Member Author

Last version

capture d ecran 2017-05-29 a 15 21 31

j0k3r
j0k3r approved these changes May 30, 2017
View reviewed changes
Copy link
Member

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just one little thing, I found the padding around each tag a bit too much.

@nicosomb
Copy link
Member Author

I reduced it, it was worst before.

On this PR, I need help from @tcitworld to fix mobile view (he will fix it after his nap 💤 )

@Rurik19
Copy link
Contributor

Rurik19 commented May 30, 2017

How it looks if there are many tags, when they don't fit in one line?

@tcitworld
Copy link
Member

How it looks if there are many tags, when they don't fit in one line?

It's limited to 3 tags.

@Rurik19
Copy link
Contributor

Rurik19 commented May 30, 2017 •
edited

It's limited to 3 tags.

But article can have more tags? Should to have "... x more" pseudo-tag with popup of some sort with other tags?

@nicosomb
Copy link
Member Author

But article can have more tags? Should to have "... x more" pseudo-tag with popup of some sort with other tags?

Sure. But in an other PR 😋

@j0k3r
Copy link
Member

j0k3r commented May 30, 2017

Are we good to go then?

@nicosomb
Copy link
Member Author

Nope. Still waiting the end of @tcitworld's nap.

@nicosomb
Copy link
Member Author

I moved the domain name

With this fix, mobile view is better. Not perfect, but better.
We can merge.

@tcitworld
Copy link
Member

There seem to have issues...
selection_077

@j0k3r
Copy link
Member

j0k3r commented May 31, 2017

Only when images aren't displayed?

@tcitworld
Copy link
Member

tcitworld commented May 31, 2017 •
edited

Nope. Missing pictures are always outputted as <img src="" /> for some reason.

@tcitworld
Copy link
Member

selection_078

@nicosomb
Copy link
Member Author

Oh Did I forget a check on preview field? 😥

@tcitworld
Add some flex magic
91775f3 
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@tcitworld
Copy link
Member

selection_081

@nicosomb
Copy link
Member Author

Still broken

capture d ecran 2017-05-31 a 15 21 30
capture d ecran 2017-05-31 a 15 23 26

@tcitworld
Copy link
Member

Still broken

If you're in dev environment, you need to do yarn run build:dev.

@aaa2000 aaa2000 mentioned this pull request May 31, 2017
Merged
@nicosomb nicosomb merged commit 757ec83 into 2.3 May 31, 2017
@nicosomb nicosomb deleted the add-tags-list-view branch May 31, 2017 19:18
@j0k3r j0k3r mentioned this pull request May 31, 2017
Merged
This was referenced May 31, 2017
Closed
Closed
@tcitworld
Copy link
Member

They're still a little too big, I'll try improving that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j0k3r j0k3r j0k3r approved these changes

@Rurik19 Rurik19 Awaiting requested review from Rurik19

@tcitworld tcitworld Awaiting requested review from tcitworld

Assignees

@tcitworld tcitworld

Labels
Ready for review
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nicosomb @tcitworld @Rurik19 @j0k3r