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 default text for back-link component #793

Merged
merged 3 commits into from
Jun 15, 2018

Conversation

web-bert
Copy link
Contributor

If you don't specify either html or text params there would be no text output
This adds default text of 'Back' in this case so you can call with only an href param
Closes issue #792

@NickColley
Copy link
Contributor

Hey! This is looking great!

If you are able would you mind updating the examples in the review application to take advantage of this default?

https://github.com/alphagov/govuk-frontend/search?utf8=%E2%9C%93&q=govukBackLink&type=

I have also raised an issue for us to do the same in the guidance: alphagov/govuk-design-system#326

Don't worry if not, we can do this for you.

@NickColley
Copy link
Contributor

Thank you for updating the CHANGELOG, we'll likely get this in the next release after #791

So you'll need to rebase once that is done, or we can clean that up for you if you want :)

@web-bert
Copy link
Contributor Author

@NickColley I hope that is ok

@NickColley
Copy link
Contributor

NickColley commented Jun 14, 2018

Awesome thanks so much!

Keep an eye on #791, once we've finished the new release we'll get this merged as part of the next release (which is likely to be 1.0.0) (and rebase the CHANGELOG addition)

@NickColley
Copy link
Contributor

NickColley commented Jun 15, 2018

Hey!
We'll need to update your CHANGELOG update to move it into an unreleased section like below, I can do this for you if you want, let me know.

# Changelog

Note: We're not following semantic versioning yet, we are going to talk about this soon.

## Unreleased

 🆕 New features:

- Add default text for back-link component
  ([PR #793](https://github.com/alphagov/govuk-frontend/pull/793))

@web-bert
Copy link
Contributor Author

Ok, I wasn't sure where it should live, I'll try and put it in the right place

If you don't specify either html or text params there would be no text output
This adds default text of 'Back' in this case so you can call with only an href param
Closes issue alphagov#792
@web-bert
Copy link
Contributor Author

@NickColley I have rebased and force pushed, does that look ok?

@NickColley
Copy link
Contributor

@web-bert we try to put the changelog in order of importance, so we'd put the new features above 'internal' changes.

Sorry this isn't documented, I'll make sure to fix our documentation so someone doesnt run into this again.

@web-bert
Copy link
Contributor Author

Pushed a fixup.

Maybe it would be a good idea to create the "groups" in there by default with "None" for each one initially, so they are in the right order? This also serves to say you have made none of a specific type of change if no one removes the none and adds a message?

@NickColley
Copy link
Contributor

@web-bert good idea, I'll ping you for the review of this docs update I'm working on thanks :)

@NickColley
Copy link
Contributor

The fixup looks great, if you squash that into the changelog commit we'll be good to go.

@web-bert
Copy link
Contributor Author

Done

@NickColley
Copy link
Contributor

NickColley commented Jun 15, 2018

We're close but need to remove 38c9e4b from the history.

I can do this for you when I merge it in if you'd prefer since it's a pain?

Now that there is a default text of 'Back' there is no need for the param
This removes the text param from the examples
@web-bert
Copy link
Contributor Author

I have merged the two commits, not sure how to remove the commit, so if you can do that on merge that would be great.

Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Thanks for the effort to get the commits right!

This looks great, thanks so much for helping us make GOV.UK Frontend better.

If someone else from the GOV.UK Design System team reviews and approves this we can get it merged and into the next release. 👍

Copy link

@kr8n3r kr8n3r left a comment

Choose a reason for hiding this comment

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

looks good, thanks

@web-bert
Copy link
Contributor Author

No worries, I expected a bit of effort as I'm not familiar with the project as a contributor. It's all a team effort, thanks for the pointers to get it ready for merge - I look forward to seeing it in the next release.

@NickColley NickColley merged commit 09fc7a8 into alphagov:master Jun 15, 2018
This was referenced Jun 21, 2018
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.

3 participants