Skip to content
This repository has been archived by the owner on Jun 9, 2022. It is now read-only.

Carousel ui #14

Merged
merged 6 commits into from
Dec 3, 2015
Merged

Carousel ui #14

merged 6 commits into from
Dec 3, 2015

Conversation

AdaRoseCannon
Copy link
Contributor

Fixes for #2 and some improvements on #1 which is
solved already so should be closed but this pr
makes a few further improvements.

  • Starts off with only 3 fields and adds more as needed.
  • Tweak appearance and copy
  • Can now remove rows
  • Pasting a carousel appends to whatever is there already allowing easy concatenation of items

</div>
</FORM>
</P>
<FORM id="carouselForm" action="/generators/carousel" method="get">
<DIV><I>(A descriptive name for when this carousel<BR/>is listed the admin screen)</I></DIV>
<div class="url-and-duration o-forms-group">
<DIV><I>(A descriptive name for when this carousel is listed the admin screen)</I></DIV>
Copy link
Contributor

Choose a reason for hiding this comment

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

(A descriptive name for when this carousel is listed in the admin screen)

@JakeChampion
Copy link
Contributor

LGTM (Looks good to me) 👍
@seanmtracey Could you comment whether you approve of this pull request? :-)

@seanmtracey
Copy link
Contributor

If I generate a carousel URL, and then copy that URL and paste it into the paste URL box at the top of th admin without reloading the page, the form appends the items to existing carousel, rather than building it from scratch, effectively duplicating the carousel within itself.

@AdaRoseCannon
Copy link
Contributor Author

That is intentional behaviour allowing easy adding one carousel to another. I am also addressing the following concerns:

Clear paste box on paste
Prevent pressing return

@seanmtracey
Copy link
Contributor

Cool, happy to give it another look over when you've got that done.

JakeChampion added a commit that referenced this pull request Dec 3, 2015
@JakeChampion JakeChampion merged commit 05c3b43 into master Dec 3, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants