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

Closes #340: New modal for messages #341

Closed
wants to merge 6 commits into from

Conversation

ThomasLuu00
Copy link

@ThomasLuu00 ThomasLuu00 commented Nov 16, 2019

Description

I added a modal for when a message is clicked that shows all the details for the message. It still requires some styling but the functionality is there.

Fixes #340

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated changelog.md with changes made

@davidmckenzie
Copy link
Member

Nice one, looks like this includes the changes from #339 as well though. Feel free to submit with only the modal changes, as I suspect we'll need to rework #339 a bit before it merges in due to the issues with colspan on the headers, and formatting issues

@DanrwAU DanrwAU changed the base branch from master to v0.3.6 November 17, 2019 05:56
@DanrwAU
Copy link
Member

DanrwAU commented Nov 17, 2019

Changed the base branch to v0.3.6 - we don't merge directly to master.

Copy link
Member

@marshyonline marshyonline left a comment

Choose a reason for hiding this comment

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

server/public/templates/myModalContent.html is too generic of a name.

@marshyonline
Copy link
Member

Other than the outlined issues - I really like the idea and am keen to see it implemented

@ThomasLuu00
Copy link
Author

I have removed the old commits from #339 and changed the modal file name as requested.

@ThomasLuu00
Copy link
Author

Okay, I think I messed up the rebase so it's not working, will try to fix it again.

@ThomasLuu00
Copy link
Author

Fixed the problem, works now

Copy link
Member

@marshyonline marshyonline left a comment

Choose a reason for hiding this comment

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

I feel like we can clean this up a bit
image

It is not respecting "Hide Source"
It seems to be respecting "Hide Capcode" but is showing an empty field -
I feel Source and Capcode should be removed from the box if these options are enabled
@DanrwAU & @davidmckenzie Thoughts?

Formatting is easy, I can add this once the above is good

@marshyonline
Copy link
Member

Oh and please add your change to the CHANGELOG :)

@DanrwAU
Copy link
Member

DanrwAU commented Nov 20, 2019

It is not respecting "Hide Source"
It seems to be respecting "Hide Capcode" but is showing an empty field -
I feel Source and Capcode should be removed from the box if these options are enabled
@DanrwAU & @davidmckenzie Thoughts?

Agreed needs to hide these fields if they are set to be hidden. Also needs some prettiness, that's not my wheelhouse tho

@ThomasLuu00
Copy link
Author

If someone has an idea of how it should look, I will add the style changes. But yes, I will add some changes to respect the hidden columns settings.

@ThomasLuu00
Copy link
Author

I have updated the changelog and implemented the hidesource and hidecapcode changes.
Screen Shot 2019-11-20 at 6 39 55 PM

@s3m1s0n1c
Copy link
Member

Is there an option to turn it off as I probably wouldn't want this on my site..

Sorry to be annoying..

Thanks
Sonic

@davidmckenzie
Copy link
Member

@s3m1s0n1c it should only display on mobile view when columns are truncated, where it would be useful. May consider moving it to an unobtrusive button pinned to the right instead of from clicking the row

I'll take a look at prettifying this after the oracle saga is over :)

@marshyonline
Copy link
Member

If this is for mobile only - I was seeing the Modal on a desktop

@eopo
Copy link
Collaborator

eopo commented Nov 21, 2019

How about accordion view. We could even enable them on desktop to view maps or other stored plugin data 🤔

@marshyonline
Copy link
Member

+1 for @eopo's idea

@davidmckenzie
Copy link
Member

oooh accordian is nice, good idea, kinda like what I started on with #167 / #171 but never got around to finishing

@ThomasLuu00
Copy link
Author

I added the change to make the modal so it only appears for mobile but not small browsers. But based on the discussion here, seems like an accordian view is more popular. I can work on that view as well, should I open up a new issue?

@DanrwAU
Copy link
Member

DanrwAU commented Jan 25, 2020

Is this one cancelled in favour of #348 ?

@DanrwAU DanrwAU changed the base branch from v0.3.6 to v0.3.7 March 18, 2020 23:40
@DanrwAU DanrwAU changed the base branch from v0.3.7 to v0.3.8 April 21, 2020 10:24
@DanrwAU DanrwAU changed the base branch from v0.3.8 to v0.3.9 May 4, 2020 06:18
@DanrwAU DanrwAU added this to the 0.3.9 milestone May 4, 2020
@DanrwAU DanrwAU changed the base branch from v0.3.9 to v0.3.10 May 13, 2020 09:43
@DanrwAU DanrwAU closed this May 17, 2020
@DanrwAU DanrwAU reopened this May 17, 2020
@DanrwAU DanrwAU changed the base branch from v0.3.10 to develop June 24, 2020 11:01
@DanrwAU
Copy link
Member

DanrwAU commented Dec 21, 2022

Abandonned - no commits in 3 years

@DanrwAU DanrwAU closed this Dec 21, 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.

None yet

6 participants