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

#1017 : Adding visible announcements button for easy visibility #1022

Merged
merged 5 commits into from
Aug 10, 2019

Conversation

abhiramr
Copy link

@abhiramr abhiramr commented Aug 9, 2019

Issue Reference

This PR addresses the Issue : Fixes part of #1017
#1017

Summary

dded new Red button for Announcements on top for easy access and visibility per #1017 .

@abhiramr
Copy link
Author

abhiramr commented Aug 9, 2019

@tomahawk-pilot I've tested it locally, if that helps.

@subins2000
Copy link

I've tested this locally. Here's how it looks :
Screenshot_2019-08-10_01-10-18

@dauntlessnomad
Copy link
Member

@subins2000 is verified

@dauntlessnomad
Copy link
Member

@biswaz please verify and merge this

@abhiramr
Copy link
Author

abhiramr commented Aug 9, 2019

This satisfies #1017 with hashtags that can be cleared and added while adding a new announcement -

hashtags1
hashtags2
hashtags3

@abhiramr
Copy link
Author

abhiramr commented Aug 9, 2019

Here's how it looks when a tag has been selected -

Screenshot from 2019-08-10 03-29-50

@jojurajan
Copy link

jojurajan commented Aug 10, 2019

@abhiramr Could you take a look at #1027 and merge the changes to your PR if applicable?

Please ignore, if not required.

@abhiramr
Copy link
Author

@jojurajan I've looked at them already. My PR covers all of it and a little more and unfortunately renders #1027 superfluous. I really appreciate his work though :)

@abhiramr
Copy link
Author

@tomahawk-pilot This still hasn't been merged no?

@abhiramr
Copy link
Author

@subins2000 Surprising. Do you have my complete code? Have migrations been rerun?

@subins2000
Copy link

@abhiramr Never mind. It works. It didn't work cause no tags were added.

Note to sysadmin : Make sure atleast one announcement item has a tag after merging this, otherwise the whole page will fail

Screenshot_2019-08-10_12-40-10

@abhiramr
Copy link
Author

@subins2000 Thanks for this test. Will try to add a case for 0 tags. Ideally shouldn't fail because of no data.

@dauntlessnomad
Copy link
Member

is the model modified to add the tags
@subins2000

@abhiramr
Copy link
Author

@tomahawk-pilot Yes it is.

@subins2000
Copy link

@tomahawk-pilot yes, the model has been modified by @abhiramr

@subins2000
Copy link

There seems to be a conflict now

#Get unique hashtags from items in DB
def get_hashtags(announcement_obj):
hashtags_str = ""
for i in (announcement_obj.objects.all().values_list('hashtags', flat=True)):
Copy link

Choose a reason for hiding this comment

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

why are you converting a list into a string and then back into a list again ?

for i in (announcement_obj.objects.all().values_list('hashtags', flat=True)):
if i !='':
hashtags_str = hashtags_str +","+i
hashtags = list(set([j.strip() for j in hashtags_str.strip(',').split(',')]))
Copy link

Choose a reason for hiding this comment

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

similarly list into a set and then into a list again

@biswaz biswaz merged commit 4db7aee into raksha-life:master Aug 10, 2019
@abhiramr
Copy link
Author

I'll give you reasons in a bit. For now can you edit any single announcement with a tag? I see a 500 on the page. @biswaz

@biswaz
Copy link

biswaz commented Aug 10, 2019

@abhiramr Yeah I was in the process of doing it. It's done now. We could've actually used the taggit lib. But if it works.. it works :)

@sreenadh
Copy link

How about using a plugin like bootstrap-tagsinput along with typeahead for the tag input field.

https://bootstrap-tagsinput.github.io/bootstrap-tagsinput/examples/ see the example with typeahead. Make sure that it doesn't compromise the UX on small screens

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants