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

Feature/apply media design and links #4285

Merged
merged 25 commits into from Oct 25, 2018

Conversation

isaacmg410
Copy link
Contributor

@isaacmg410 isaacmg410 commented Oct 16, 2018

🎩 What? Why?

This PR adds the new design of Uploaded Attachments to a Conference, and add the MediaLinks entity

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG entry
  • Add MediaLink enitiy
  • Add/modify seeds
  • Add tests
  • Add design of media and links

📷 Screenshots (optional)

Description

screenshot-localhost-3000-2018 10 23-11-17-14

@ghost ghost assigned isaacmg410 Oct 16, 2018
@ghost ghost added the status: WIP label Oct 16, 2018
@ghost ghost added the status: WIP label Oct 18, 2018
@isaacmg410
Copy link
Contributor Author

@decidim/lot-core CircleCI tests have finished, but test checks does not show it correctly. "upload_coverage" say "Runing", but when you go to CircleCi page, it is "succeedded"
Are you aware of this?

@mrcasals
Copy link
Contributor

@isaacmg410 yes, we already notified CircleCI about this.

@isaacmg410
Copy link
Contributor Author

@mrcasals okey! then, can this PR be reviewed?

@mrcasals
Copy link
Contributor

@isaacmg410 there are some conflicts now, can you check them please?

@ghost ghost added the status: WIP label Oct 19, 2018
@isaacmg410
Copy link
Contributor Author

@mrcasals conflicts solved!

@isaacmg410
Copy link
Contributor Author

@decidim/lot-core It says that all tests are waiting for status, but, when you go to https://circleci.com/workflow-run/eaa3cec9-678c-49ec-acaf-dfad8c701102?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link all testes are green

@ghost ghost added the status: WIP label Oct 23, 2018
@isaacmg410
Copy link
Contributor Author

@mrcasals requested changes applied. 😄

@ghost ghost added the status: WIP label Oct 23, 2018
@isaacmg410
Copy link
Contributor Author

@decidim/lot-core ready to be merged

@@ -0,0 +1,10 @@
<% if model.any? %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this logic to the show method of the cell

include Decidim::ApplicationHelper
include Decidim::SanitizeHelper

def show
Copy link
Contributor

Choose a reason for hiding this comment

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

Add return unless models.any? here and remove it from the views

@form = form(Decidim::Conferences::Admin::MediaLinkForm).instance
end

def create
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this action is not tested, can you add a system spec for this please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ups. I have forgotten

uri = URI.parse(link)
errors.add :link, :invalid if !uri.is_a?(URI::HTTP) || uri.host.nil?
rescue URI::InvalidURIError
errors.add :link, :invalid
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add tests for this case please?

es: "Media Link es",
ca: "Media Link ca"
)
# fill_in_i18n(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this

@ghost ghost added the status: WIP label Oct 24, 2018
@isaacmg410
Copy link
Contributor Author

@decidim/lot-core requested changes applied 😄

@isaacmg410 isaacmg410 changed the title Feature/apply media design and links [WIP] Feature/apply media design and links Oct 24, 2018
@ghost ghost added the status: WIP label Oct 24, 2018
@isaacmg410 isaacmg410 changed the title [WIP] Feature/apply media design and links Feature/apply media design and links Oct 24, 2018
@mrcasals
Copy link
Contributor

@isaacmg410 rubocop is complaining, can you check it? 😄

@ghost ghost added the status: WIP label Oct 25, 2018
@isaacmg410
Copy link
Contributor Author

@mrcasals done

@mrcasals mrcasals merged commit b547c31 into master Oct 25, 2018
@mrcasals mrcasals deleted the feature/apply_media_design_and_links branch October 25, 2018 08:54
@ghost ghost removed the status: WIP label Oct 25, 2018
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.

None yet

2 participants