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

Add old-style rendering option to progress bars #1329

Merged
merged 5 commits into from Aug 30, 2016
Merged

Conversation

wch
Copy link
Collaborator

@wch wch commented Aug 30, 2016

Closes #1327. This adds an argument, style, to withProgress and Progress$new, which lets the user choose new notification-based progress bars with "notification" (the default), or the older style progress bars with "old".

This is so that users who have custom-styled their progress bars with CSS can keep that CSS. It will require them to add the style="old" argument, however.

We had also discussed adding a "raw" option where the progress bars would get CSS classes but we wouldn't actually have any CSS that styles the bars. I think it would be better to wait on that until the next version of Shiny.

@wch wch added the review label Aug 30, 2016
* Progress indicators now use the notification API. (#1160)
* Progress indicators can now either use the new notification API, using
`style="notification"`, or be displayed with the previous styling, using'
`style="old"`. (#1160, #1327, #1329)
Copy link
Member

@jcheng5 jcheng5 Aug 30, 2016

Choose a reason for hiding this comment

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

Should be moved to "breaking changes" section and reworded to make clear that the default behavior is the new style. And probably specifically call out that if you have custom CSS for the old styling then you'll need to use old to not get broken UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can take care of that, since I'm working on NEWS. You can just undo the change to NEWS, @wch if you want (or we can work it out later because there will be a merge conflict otherwise).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll take care of it here, since it makes sense to put it in this PR. I'll probably need to add something about it to the release notes after it's merged.

@jcheng5 jcheng5 merged commit 76ffc20 into master Aug 30, 2016
@jcheng5 jcheng5 deleted the progress-compatibility branch August 30, 2016 21:50
@jcheng5 jcheng5 removed the review label Aug 30, 2016
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

3 participants